Skip to content

HDDS-3130. Add jaeger trace span in s3gateway #645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

runzhiwang
Copy link
Contributor

@runzhiwang runzhiwang commented Mar 6, 2020

What changes were proposed in this pull request?

Add jaeger trace span in s3gateway.

What is the link to the Apache JIRA

https://jira.apache.org/jira/browse/HDDS-3130

How was this patch tested?

The jaeger UI shows the right trace information.
image

@runzhiwang
Copy link
Contributor Author

@elek Could you help review this patch ? Thank you very much.

@elek
Copy link
Member

elek commented Mar 11, 2020

Thank you very much the idea and the patch @runzhiwang I 100% support to introduce tracing for s3g. It's a very good idea.

About the implementation: It's definitely a step forward but I started to think how is it possible to do in a transparent way without duplication. I check if the resource name is available from some generic filter and it seems to be possible.

My proposal is to do it from a central place, something like this:

@Provider
public class TracingFilter implements ContainerRequestFilter,
    ContainerResponseFilter {

  public static final String TRACING_SCOPE = "TRACING_SCOPE";

  @Context
  private ResourceInfo resourceInfo;

  @Override
  public void filter(ContainerRequestContext requestContext)
      throws IOException {
    Scope scope = GlobalTracer.get().buildSpan(
        resourceInfo.getResourceClass().getSimpleName() + "." + resourceInfo
            .getResourceMethod().getName()).startActive(true);

    requestContext.setProperty(TRACING_SCOPE, scope);
  }

  @Override
  public void filter(ContainerRequestContext requestContext,
      ContainerResponseContext responseContext) throws IOException {

    Object scope = requestContext.getProperty(TRACING_SCOPE);
    if (scope != null) {
      ((Scope) scope).span().finish();
    }
  }
}

What do you think? I think it would simplify this patch (you don't need to modify all the methods...)

@runzhiwang
Copy link
Contributor Author

@elek Very reasonable suggestion. I will try it, thank you very much.

@runzhiwang runzhiwang force-pushed the s3g-jaeger branch 2 times, most recently from 841d645 to b34adaa Compare March 16, 2020 02:59
@runzhiwang runzhiwang closed this Mar 16, 2020
@runzhiwang runzhiwang reopened this Mar 16, 2020
@runzhiwang runzhiwang force-pushed the s3g-jaeger branch 2 times, most recently from c24a897 to 79f94d8 Compare March 16, 2020 03:42
@runzhiwang runzhiwang closed this Mar 16, 2020
@runzhiwang runzhiwang reopened this Mar 16, 2020
@runzhiwang
Copy link
Contributor Author

runzhiwang commented Mar 16, 2020

@elek Hi, I have updated PR according to you suggestion, thank you very much. Additionally, if throw exception when execute request, the method public void filter(ContainerRequestContext requestContext, ContainerResponseContext responseContext) will not be executed, and span can not be finished, then the following jaeger span will have a error parent, just as the image shows. So I finish the old span before build a new span in closeActiveScope. Besides, scope.close also can not be ignored.
image

@runzhiwang runzhiwang force-pushed the s3g-jaeger branch 4 times, most recently from 4bb16cb to 18842fd Compare March 16, 2020 06:22
@runzhiwang runzhiwang closed this Mar 16, 2020
@runzhiwang runzhiwang reopened this Mar 16, 2020
@runzhiwang runzhiwang closed this Mar 18, 2020
@runzhiwang runzhiwang reopened this Mar 18, 2020
@runzhiwang
Copy link
Contributor Author

The CI failure has nothing to do with this PR.

@runzhiwang
Copy link
Contributor Author

@elek CI passed.

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

+1 Thanks the update @runzhiwang

Tested and worked well. Will merge it soon.

@elek elek closed this in 6384b8d Apr 2, 2020
@runzhiwang runzhiwang deleted the s3g-jaeger branch April 20, 2020 06:45
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