-
Notifications
You must be signed in to change notification settings - Fork 731
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
[HttpClient] Remove OTEL_SEMCONV_STABILITY_OPT_IN #5068
[HttpClient] Remove OTEL_SEMCONV_STABILITY_OPT_IN #5068
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5068 +/- ##
==========================================
- Coverage 83.69% 83.49% -0.20%
==========================================
Files 296 295 -1
Lines 12485 12397 -88
==========================================
- Hits 10449 10351 -98
- Misses 2036 2046 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM.
Do you want to remove the unused http client based entries from SemanticConventions.cs
?
@rajkumar-rangaraj Will do separately: #5071 |
@@ -275,19 +250,11 @@ public void OnStopActivity(Activity activity, object payload) | |||
activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode)); |
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.
Could you hardcode the kind to ActivityKind.Client
here as well similar to how you have done it for the webrequest one?
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.
in web request we cannot rely on activity being non-null always. here we can, but I can update this one as well later.
@utpilla @rajkumar-rangaraj @vishweshbankwar |
@wangzlei Please refer to FAQ of this announcement |
Fixes #
Design discussion issue #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes