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

fix: 2389- replaced logger unformatted strings with template literals #2499

Merged

Conversation

PaurushGarg
Copy link
Member

Which problem is this PR solving?

  • Good First issue bug-fix for OpenTelemetry-JS issue 2389.
  • Logger was using printf style formatting, which was resulting into unformatted strings (%s, %d).

Short description of the changes

  • All instances of such unformatted strings were searched and were replaced with Template Literals to resolve the issue.

@PaurushGarg PaurushGarg requested a review from a team September 25, 2021 03:07
@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #2499 (7b134c6) into main (b66c650) will increase coverage by 1.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2499      +/-   ##
==========================================
+ Coverage   93.23%   94.47%   +1.23%     
==========================================
  Files         137       73      -64     
  Lines        5042     2279    -2763     
  Branches     1066      504     -562     
==========================================
- Hits         4701     2153    -2548     
+ Misses        341      126     -215     
Impacted Files Coverage Δ
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 90.90% <100.00%> (ø)
...elemetry-exporter-zipkin/src/platform/node/util.ts 96.66% <100.00%> (ø)
packages/opentelemetry-sdk-trace-base/src/Span.ts 100.00% <100.00%> (ø)
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts
...entelemetry-instrumentation-grpc/src/grpc/index.ts
...y-instrumentation-http/src/enums/AttributeNames.ts
...emetry-instrumentation-grpc/src/instrumentation.ts
...es/opentelemetry-instrumentation-grpc/src/utils.ts
...metry-instrumentation-grpc/src/grpc/serverUtils.ts
...pentelemetry-sdk-metrics-base/src/CounterMetric.ts
... and 57 more

@vmarchaud vmarchaud linked an issue Sep 25, 2021 that may be closed by this pull request
2 tasks
@vmarchaud vmarchaud added the bug Something isn't working label Sep 25, 2021
@legendecas
Copy link
Member

IIUC, the placeholder style strings in loggers are designed to prevent generating redundant strings when the logger is not enabled. i.e. when debug level is disabled, the formatting can be evacuated and no new strings were generated in the case. Using JavaScript template strings in these places generates one-shot, unused strings when the diag logger is not enabled.

@dyladan
Copy link
Member

dyladan commented Sep 28, 2021

IIUC, the placeholder style strings in loggers are designed to prevent generating redundant strings when the logger is not enabled. i.e. when debug level is disabled, the formatting can be evacuated and no new strings were generated in the case. Using JavaScript template strings in these places generates one-shot, unused strings when the diag logger is not enabled.

That does not really matter if it doesn't work. It seems the original author expected the %s tokens to be replaced with the rest arguments as is done in other logging frameworks, but we just print all arguments without doing such formatting.

logger.debug('print this %s', 'string') // print this %s string

This PR fixes a bug. Deferring the string building to later can be taken on as a future enhancement (possibly by adding logf type methods).

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

@dyladan: Deferring the string building to later can be taken on as a future enhancement (possibly by adding logf type methods).

Sounds good to me. I'm 👍 on this PR if we are going to add the deferred string building in follow-up PRs.

@dyladan
Copy link
Member

dyladan commented Sep 29, 2021

@PaurushGarg please update the branch so it can be merged

@PaurushGarg
Copy link
Member Author

@PaurushGarg please update the branch so it can be merged

@dyladan Thanks. I updated the branch, as suggested.

@dyladan
Copy link
Member

dyladan commented Sep 29, 2021

This test failure is my fault and will be fixed with #2511

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger prints unformatted strings after ComponentLogger addition
5 participants