Skip to content

Basic Jaeger Tracing Added#1

Open
suab321321 wants to merge 22 commits intomasterfrom
suab321321_jaegerTracing
Open

Basic Jaeger Tracing Added#1
suab321321 wants to merge 22 commits intomasterfrom
suab321321_jaegerTracing

Conversation

@suab321321
Copy link
Owner

@suab321321 suab321321 commented Mar 15, 2020

Signed-off-by: Abhinav Singh singhabhinav9051571833@gmail.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
…321/ceph into suab321321_jaegerTracing

Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
@suab321321 suab321321 force-pushed the suab321321_jaegerTracing branch from a54fd8a to 05e1442 Compare March 15, 2020 09:47
@suab321321 suab321321 force-pushed the suab321321_jaegerTracing branch 4 times, most recently from 05e1442 to d0a8211 Compare April 22, 2020 19:37
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
@suab321321 suab321321 force-pushed the suab321321_jaegerTracing branch from d0a8211 to eeebe21 Compare April 22, 2020 19:40
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
*env.auth_registry, &client_io, env.olog,
null_yield, scheduler.get() ,&http_ret);
int ret;
#ifdef WITH_JAEGER

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of having function definition changed everywhere with 2 alternatives, can you try testing adding span to RGWSimpleRequest https://github.com/ideepika/ceph/blob/jaeger-rebase-8.0/src/rgw/rgw_rest_client.h#L10:L10


int RGWDeleteObj_ObjStore_SWIFT::verify_permission(Jager_Tracer& tracer, const Span& parent_span)
{
Span span = tracer.child_span("rgw_rest_swift.cc RGWDeleteObj_ObjStore_SWIFT::verify_permission", parent_span);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mentioned you were going to try using a global tracer, can you explain your approach here?

Copy link
Owner Author

@suab321321 suab321321 May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ideepika the tracer is global, but the spans are not, each "Root" span denotes a new client request and that "Root" span will contain its serveral follow_up spans and child spans.
In the above example I m creating a new child span, parent_span is the span created in the function which is calling this function RGWDeleteObj_ObjStore_SWIFT::verify_permission

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suab321321 I understand that, my question is : is it important to pass the tracer to the function, or are you keeping the tracer globally unique for rgw?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ideepika yes it is mandatory to pass the tracer as an argument, even if I make the tracer an extern and use this as global tracer, but I wont be able to make the Span an extern variable.I understand that by making and tracer and Span global I dont have to pass the variable as argument which in turn will prevent the creating of overloaded function, but the span variable is an issue here

Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Copy link

@ideepika ideepika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @suab321321 !
Nice work till now, a few suggestions:

  1. can you cleanup commits, with eliminating changes which are unrelated to jaeger.
  2. can you also add a screenshot of jaeger trace with each commit addition, that way it would be easier to follow your code updates.
  3. we can test the span traversal using req_state as we discuss, since that would change a lot of this code, I would rather help you out in that first, let's keep review stalled uptil then.

@@ -6725,7 +6725,7 @@ int RGWDeleteObj::verify_permission()
int RGWDeleteObj::verify_permission(Jager_Tracer& tracer, const Span& parent_span)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Jager_Tracer/Jaeger_Tracer

int RGWListBuckets::verify_permission(Jager_Tracer& tracer, const Span& parent_span)
{
Span span = tracer.child_span("rgw_op.cc RGWListBuckets::verify_permission", parent_span);
rgw::Partition partition = rgw::Partition::aws;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove non-jaeger changes in the further cleanups, it would make work easier for reviewer, hence better focus on feedback.

Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
{
Span span=tracer.child_span("common_init.cc common_preinit()",parent_span);
// set code environment
ANNOTATE_BENIGN_RACE_SIZED(&g_code_env, sizeof(g_code_env), "g_code_env");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes part of your code? If not can you clean them up?

std::unique_ptr<opentracing::Span> childSpan(const char *, const std::unique_ptr<opentracing::Span> &);
std::unique_ptr<opentracing::Span> followUpSpan(const char *, const std::unique_ptr<opentracing::Span> &);
~jTracer(){
opentracing::Tracer::Global()->Close();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

}

#ifdef WITH_JAEGER
void global_pre_init(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove unrelated changes, it would really help in reviewing your work better + faster

@@ -0,0 +1,64 @@
#ifndef TRACER_H_

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there are two files src/common/jaegerTracer and this one, can you move them to src/common/JaegerTracer{.h/cc}

suab321321 pushed a commit that referenced this pull request Aug 27, 2020
Changes addressing comments in PR - commit to be
squashed prior to merge

Signed-off-by: Paul Cuzner <pcuzner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants