-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat(@opentelemetry-instrumentation-http): support adding custom attributes before a span is started #2332
feat(@opentelemetry-instrumentation-http): support adding custom attributes before a span is started #2332
Conversation
|
e78c15d
to
5afff81
Compare
5afff81
to
62c7099
Compare
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
Codecov Report
@@ Coverage Diff @@
## main #2332 +/- ##
==========================================
- Coverage 92.77% 92.76% -0.02%
==========================================
Files 145 145
Lines 5221 5226 +5
Branches 1070 1071 +1
==========================================
+ Hits 4844 4848 +4
- Misses 377 378 +1
|
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.
Please describe the new options in the readme of the package.
export interface StartOutgoingSpanCustomAttributeFunction { | ||
(request: RequestOptions ): SpanAttributes; | ||
} | ||
|
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 the specs meeting it looks like there will be a different way to doing this (if I'm reading and listening correctly)
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.
I believe i read it the same as you, this is welcome as a lot of instrumentation had some kind of system like this already. I believe this should be handled in the base instrumentation class though. I think it make sense to create a dedicated issue for this
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.
👍
Before it's handled in the base instrumentation, will these startHooks for http be accepted? For I wanna use some custom fields to make the sample decision in my project. :)
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.
I think so
done :) |
@echoontheway Could you fix the conflicts ? |
774d8d9
to
bcd3206
Compare
…ibutes before a span is started
bcd3206
to
1c5221a
Compare
done,sorry for late :) |
Which problem is this PR solving?
Short description of the changes