-
Notifications
You must be signed in to change notification settings - Fork 302
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
Updated MassTransit instrumentation to OpenTelemetry 1.0 #85
Updated MassTransit instrumentation to OpenTelemetry 1.0 #85
Conversation
One test, related to trace filtering is failing (I will investigate why). But the rest of the PR is ready to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from the perspective of removing ActivitySourceAdapter and leveraging AddLegacySource feature.
Not MT expert to offer deeper review :)
test/OpenTelemetry.Contrib.Instrumentation.MassTransit.Tests/MassTransitInstrumentationTests.cs
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #85 +/- ##
==========================================
- Coverage 77.55% 77.52% -0.03%
==========================================
Files 51 52 +1
Lines 1430 1455 +25
==========================================
+ Hits 1109 1128 +19
- Misses 321 327 +6
|
@cijothomas any blockers for this PR? |
Would you update codeowners file to list yourself as an approver for the MassTransit instrumentation? Similar to this https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/CODEOWNERS#L12 |
@cijothomas done |
} | ||
|
||
public override void OnStopActivity(Activity activity, object payload) | ||
{ | ||
if (this.options.TracedOperations != null && !this.options.TracedOperations.Contains(activity.OperationName)) | ||
{ | ||
MassTransitInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: RequestIsFilteredOut
doesn't sound the apt name here.
} | ||
catch (Exception ex) | ||
{ | ||
MassTransitInstrumentationEventSource.Log.EnrichmentException(ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This error message seem to indicate an error occuring in some user defined Enrich function. Given this is an internal transformation error, please rename the method and log message accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some non-blocking comments, please try to address them in a future PR.
Also it'd be good to get a changelog.md file initiated for this project.
@cijothomas thank you. I will fix remaining comments in the next pull requests. |
@alexvaluyskiy The release is manual, and one of the maintainers have to do it now. (@CodeBlanch did a manual release for wcf last week, but we need to list the process and (then automate)) |
But I think, we have to wait for OpenTelemetry 1.0.2 release first |
yea. we'll likely name it (OpenTelemetry .SDK)1.1.0-beta1 before pushing to nuget.org. |
The release process I used for WCF... Setup
Release
|
thanks @CodeBlanch |
This PR upgrades the MassTransit instrumentation to OTel version 1.0.2