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

Instrumentation to store context object into Activity.CustomProperty #1128

Merged

Conversation

cijothomas
Copy link
Member

HttpClient, HttpWebRequest, Grpc, SqlClient, Asp.Net, Asp.NetCore instrumentations modified to

  1. Store context object (context here refers to the HttpRequest, SqlComman) in activity as a customobject. (already existing in some cases)
  2. The above is done only if AllDataRequested=true.
  3. Tests to validate this.

TODOS:
Expose the key string for these objects publicly. This makes it easier to consumers to use it.
Write example which demonstrate the usage.

Unrelated to this PR: Refactor the Instrumentation Tests. Some have basic tests, more tests. It should be reorged.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public API reviewed

@cijothomas cijothomas requested a review from a team August 21, 2020 16:53
if (!activity.IsAllDataRequested)
{
return;
}

activity.SetCustomProperty(ExceptionCustomPropertyName, exception);
Copy link
Member

Choose a reason for hiding this comment

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

In HttpWebRequestActivitySource.netfx.cs there is also Request & Response set, both before activity.IsAllDataRequested check. Did you leave them alone on purpose?

I don't think that's necessarily a bad thing. I intentionally put them all outside of the check for anyone wanting to do advanced things. Something like... make a sampling decision based on the raw requests. Or maybe have their own ActivityListeners? Not saying we must do that, I just thought it was worth the perf 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed it. I prefer to keep everything inside AllDataRequested.

My general take is to follow what open telemetry spec suggests:
Samplers get a specific set of things - name, parentcontext, kind, initial attributes, links.
we don't want to provide more anything more, unless there is a strong reason to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw - samplers dont get access to raw Actiivty itself, so even if someone want to make intelligent sampling decision, they wont be able to leverage this custom obj.

Copy link
Member

Choose a reason for hiding this comment

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

Hah ok good point!

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

Let's also get Request & Response in HttpWebRequestActivitySource.netfx.cs. Otherwise looks good.

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #1128 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1128      +/-   ##
==========================================
+ Coverage   77.33%   77.34%   +0.01%     
==========================================
  Files         220      220              
  Lines        6256     6260       +4     
==========================================
+ Hits         4838     4842       +4     
  Misses       1418     1418              
Impacted Files Coverage Δ
...umentation.AspNet/Implementation/HttpInListener.cs 92.85% <100.00%> (+0.17%) ⬆️
...tation.AspNetCore/Implementation/HttpInListener.cs 89.13% <100.00%> (+0.24%) ⬆️
...rpc/Implementation/GrpcClientDiagnosticListener.cs 89.65% <100.00%> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs 88.00% <100.00%> (ø)
...ient/Implementation/SqlClientDiagnosticListener.cs 78.46% <100.00%> (ø)
....Prometheus/PrometheusExporterMetricsHttpServer.cs 74.50% <0.00%> (-1.97%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️

@cijothomas cijothomas merged commit 7807c81 into open-telemetry:master Aug 21, 2020
@cijothomas cijothomas deleted the cijothomas/instrumentationfixes1 branch August 21, 2020 20:10
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