Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(sdk-node)!: Automatically configure logs exporter #4740

Merged
merged 19 commits into from
Aug 19, 2024
Merged

feat(sdk-node)!: Automatically configure logs exporter #4740

merged 19 commits into from
Aug 19, 2024

Conversation

mhennoch
Copy link
Contributor

Which problem is this PR solving?

Fixes #4552 #4451

Short description of the changes

See #4552. I hope changes to LoggerProviderConfig are not breaking. It is exported but I don't think anyone is using it.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@mhennoch mhennoch requested a review from a team May 28, 2024 14:09
@trentm trentm linked an issue May 31, 2024 that may be closed by this pull request
[sdk-node] allow multiple spanProcessors/logProcessors to be configured #4451
Closed
pichlermarc
pichlermarc reviewed Jun 18, 2024
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I had a few comments, but overall looks good. 🙂

experimental/packages/opentelemetry-sdk-node/package.json Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Jul 1, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.04%. Comparing base (ecc88a3) to head (4c03ca0).
Report is 55 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4740   +/-   ##
=======================================
  Coverage   91.04%   91.04%           
=======================================
  Files          89       89           
  Lines        1954     1955    +1     
  Branches      416      416           
=======================================
+ Hits         1779     1780    +1     
  Misses        175      175

@evheniyt evheniyt mentioned this pull request Jul 9, 2024
Closed
4 tasks
@mhennoch
Copy link
Contributor Author

mhennoch commented Aug 6, 2024

All comments should be addressed @trentm @pichlermarc

trentm
trentm reviewed Aug 9, 2024
View reviewed changes
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Just a few nits.

experimental/packages/opentelemetry-sdk-node/src/sdk.ts Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts Show resolved Hide resolved
trentm
trentm reviewed Aug 16, 2024
View reviewed changes
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the one discussion about calling this a breaking change (for the microscopic type change).

experimental/packages/opentelemetry-sdk-node/src/sdk.ts Show resolved Hide resolved
@mhennoch mhennoch changed the title feat(sdk-node): Automatically configure logs exporter feat(sdk-node)!: Automatically configure logs exporter Aug 16, 2024
JacksonWeber
JacksonWeber approved these changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@JacksonWeber JacksonWeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

trentm
trentm approved these changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@trentm trentm added this pull request to the merge queue Aug 19, 2024
Merged via the queue into open-telemetry:main with commit 14d086a Aug 19, 2024
19 checks passed
@pichlermarc pichlermarc mentioned this pull request Aug 22, 2024
Merged
@opentelemetrybot opentelemetrybot mentioned this pull request Aug 22, 2024
Merged
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
@mhennoch @Zirak
feat(sdk-node)!: Automatically configure logs exporter (open-telemetr…
d0f0626 
…y#4740)
@saidsef saidsef mentioned this pull request Sep 18, 2024
Merged
@micahg micahg mentioned this pull request Sep 18, 2024
Open
@danqueb danqueb mentioned this pull request Sep 19, 2024
Open
This was referenced Sep 19, 2024
Open
Open
@WontonSam WontonSam mentioned this pull request Sep 19, 2024
Open
@zx8086 zx8086 mentioned this pull request Sep 19, 2024
Open
@WontonSam WontonSam mentioned this pull request Sep 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc left review comments

@trentm trentm trentm approved these changes

@JacksonWeber JacksonWeber JacksonWeber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@mhennoch @trentm @pichlermarc @JacksonWeber