-
Notifications
You must be signed in to change notification settings - Fork 576
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
autoexport: Add support for metrics #4229
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4229 +/- ##
=======================================
- Coverage 80.8% 80.8% -0.1%
=======================================
Files 148 150 +2
Lines 10238 10245 +7
=======================================
+ Hits 8281 8284 +3
- Misses 1819 1823 +4
Partials 138 138
|
f127c39
to
155c74e
Compare
6f9d996
to
2999fd4
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.
I just made a very quick look. I would need to look deeper in the following days. Looks nice so far👍
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 very good 👍 I have found only one corner case issue.
This would be useful, but doesn't really fit with how the package is currently designed. |
// SpanOption applies an autoexport configuration option. | ||
type SpanOption = option[trace.SpanExporter] |
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.
Should this be TraceOption
?
I know we have the SpanExporter
, but I wonder if this needs to match that or the corollary to metric: trace.
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.
We can address it in a separate PR if needed.
Personally, I was thinking about renaming to SpanExporterOption
.
Let's track it in a separate issue.
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 created #4471 |
Fixes #4131.
Reorganizes autoexport into
Also reorganizes tests to line up with these layers.