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.

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

[sdk-logs] remove includeTraceContext configuration and use LogRecord context when available #3817

Merged
merged 9 commits into from
May 23, 2023

Conversation

hectorhdzg
Copy link
Member

@hectorhdzg hectorhdzg commented May 17, 2023

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #3816
Fixes #3761

Short description of the changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added unit test and validated using test app

Checklist:

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

@hectorhdzg hectorhdzg requested a review from a team May 17, 2023 18:56
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #3817 (c2746ab) into main (0471c54) will increase coverage by 0.77%.
The diff coverage is 100.00%.

❗ Current head c2746ab differs from pull request most recent head c065f7e. Consider uploading reports for the commit c065f7e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3817      +/-   ##
==========================================
+ Coverage   92.20%   92.98%   +0.77%     
==========================================
  Files         290      295       +5     
  Lines        8650     9032     +382     
  Branches     1774     1841      +67     
==========================================
+ Hits         7976     8398     +422     
+ Misses        674      634      -40     
Impacted Files Coverage Δ
...perimental/packages/sdk-logs/src/LoggerProvider.ts 98.11% <ø> (-0.04%) ⬇️
experimental/packages/sdk-logs/src/Logger.ts 100.00% <100.00%> (ø)

... and 11 files with indirect coverage changes

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Seems to me like this is fixing it in the wrong place. The logger constructor should not be interpreting undefined as false.

@hectorhdzg
Copy link
Member Author

@dyladan I can update mergeConfig method to clone the object and avoid copying over the undefined values if you thing that would be better, I wonder if similar functionality is already in here.

@llc1123
Copy link
Contributor

llc1123 commented May 18, 2023

According to the latest spec, includeTraceContext should be removed.

Related: #3761

@hectorhdzg
Copy link
Member Author

Thanks @llc1123 for bringing up the related issue, updated the code to remove the config and use LogRecord context if available.

@hectorhdzg hectorhdzg changed the title [sdk-logs] logger includeTraceContext setting not defaulting to true [sdk-logs] remove includeTraceContext configuration and use LogRecord context when available May 18, 2023
experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -16,6 +16,7 @@

export {
LoggerConfig,
LoggerProviderConfig,
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about that, I can send a different PR for this if needed, trying to avoid a long import for the config
import { LoggerProviderConfig } from "@opentelemetry/sdk-logs/build/src/types";

Copy link
Contributor

@MSNev MSNev May 18, 2023

Choose a reason for hiding this comment

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

We should not really be importing from specific folders in a package "@opentelemetry/sdk-logs/build/src/types" as this makes it very fragile. And I have had issues from some teams where this didn't work for them (it was a loooong time ago) when we accidently had this in our internal code.

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should be avoiding it

Copy link
Member

Choose a reason for hiding this comment

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

I don't see it actually used anywhere in this PR though?

Copy link
Member

Choose a reason for hiding this comment

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

Oh end users. Ignore me

experimental/packages/sdk-logs/test/common/Logger.test.ts Outdated Show resolved Hide resolved
experimental/packages/sdk-logs/test/common/Logger.test.ts Outdated Show resolved Hide resolved
@pichlermarc pichlermarc merged commit 58dbbb4 into open-telemetry:main May 23, 2023
Vunovati added a commit to pinojs/pino-opentelemetry-transport that referenced this pull request Sep 11, 2023
Vunovati added a commit to pinojs/pino-opentelemetry-transport that referenced this pull request Sep 12, 2023
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Sep 28, 2023
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Oct 3, 2023
@hectorhdzg hectorhdzg deleted the hectorhdzg/loggerconfig branch December 7, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants