Skip to content
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

[app-configuration] Adding distributed tracing #5360

Merged
merged 2 commits into from
Oct 5, 2019

Conversation

richardpark-msft
Copy link
Member

@richardpark-msft richardpark-msft commented Oct 3, 2019

Some changes, mostly just refactoring.

One thing I discovered as I was doing this was that (not sure why) the listConfigurationSettingsOptions and listRevisionsOptions weren't exposing inherited properties from RequestOptionsBase (and thus I couldn't add the spanOptions).

I fixed this by just flattening them out into their own classes which appears to have solved it and was going to happen anyways since I need to do something similar to improve doc generation.

@richardpark-msft richardpark-msft changed the title [app-configuration] Instrumenting with distributed tracing [app-configuration] Adding distributed tracing Oct 3, 2019
Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

One thing you'll need to be able to do is add the TracingPolicy from core-http as part of your request pipeline. You can see how keyvaults does that here:

requestPolicyFactories = requestPolicyFactories.concat([

The reason you want this is that this policy will create a span specific to the HTTP request (we'll add more properties to it in the future) and forwards the span context by adding the traceparent/tracestate headers to the outgoing request.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Generally looks good! 👍

Left a couple comments.

I had debated about making a helper like this (where the traced code happens inside a callback.)

I think why I shied away from it is I remember having to debug similar code in F12 where it was really annoying if you stepped into the tracing code since you had to step through goop just to get back to your function body. So you ended up having to set a breakpoint in the inner function body and continue, whereas if you do the start/end pattern the developer can simply stepOver the two helper calls.

Something to think about 😄

@richardpark-msft
Copy link
Member Author

Generally looks good! 👍

Left a couple comments.

I had debated about making a helper like this (where the traced code happens inside a callback.)

I think why I shied away from it is I remember having to debug similar code in F12 where it was really annoying if you stepped into the tracing code since you had to step through goop just to get back to your function body. So you ended up having to set a breakpoint in the inner function body and continue, whereas if you do the start/end pattern the developer can simply stepOver the two helper calls.

Something to think about 😄

I'm with you on the steppability-issue with it. I'm more worried about consistently applying the pattern. Perhaps this is something we can do some work on with the code generator at some point so these classes will be a bit cleaner.

@richardpark-msft
Copy link
Member Author

One thing you'll need to be able to do is add the TracingPolicy from core-http as part of your request pipeline. You can see how keyvaults does that here:

requestPolicyFactories = requestPolicyFactories.concat([

The reason you want this is that this policy will create a span specific to the HTTP request (we'll add more properties to it in the future) and forwards the span context by adding the traceparent/tracestate headers to the outgoing request.

Added. Check out appConfigurationClient.ts's constructor.

@richardpark-msft richardpark-msft marked this pull request as ready for review October 4, 2019 03:40
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks good! :shipit:

@richardpark-msft richardpark-msft force-pushed the richardpark_tracing branch 2 times, most recently from c3a1000 to 6691c5d Compare October 4, 2019 23:36
@richardpark-msft richardpark-msft merged commit 3b402c4 into Azure:master Oct 5, 2019
@richardpark-msft richardpark-msft deleted the richardpark_tracing branch October 5, 2019 00:36
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.

3 participants