-
Notifications
You must be signed in to change notification settings - Fork 954
feat(opentelemetry-sdk-node): set resources and log provider for experimental start #6152
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6152 +/- ##
==========================================
- Coverage 95.40% 95.40% -0.01%
==========================================
Files 317 318 +1
Lines 9440 9546 +106
Branches 2188 2218 +30
==========================================
+ Hits 9006 9107 +101
- Misses 434 439 +5
🚀 New features to boost your workflow:
|
JamieDanielson
left a comment
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.
Took a pass through this and added some notes.
| config: ConfigurationModel, | ||
| sdkOptions: SDKOptions, | ||
| resource: Resource | undefined | ||
| ): LoggerProvider | undefined { |
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.
| ): LoggerProvider | undefined { | |
| ): LoggerProvider { |
Is there a scenario where setupLoggerProvider would be undefined? setupResource only returns a Resource not undefined
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.
if you don't have any log provider on env variables or config file, there is no logger provider to be setup, so it will return undefined
setupResource have a default value for resources, this is why it doesn't have the option to return undefined
| } else if (exporter.console) { | ||
| return new ConsoleLogRecordExporter(); | ||
| } | ||
| diag.warn(`Unsupported Exporter value. Using OTLP http/protobuf.`); |
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.
🤔 If someone uses a config file but neglects to specify a valid exporter (otlp_http, otlp_grpc, otlp_file/development(soon), console)... should we default an exporter for them or should we consider it a noop because it's invalid? It's not clear to me.
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 saw the default should be otlp_http in this case, so I kept at that.
|
Note: tests will get fixed when this gets merged #6166 I copied the majority of tests form the original sdk tests, to make sure the behaviour didn't change with the new function |
Fixes #5822
Note: Still need to add tests, but can be reviewed otherwise