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

Print JSON objects to stdout without a wrapping array #2196

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Aug 20, 2021

Hi! This builds on top of #2195; please review only the second commit.

This is just an improvement suggestion of slightly personal taste. I think it's a bit nicer JSON to look at, if the objects are marshalled as { ... } { ... } { ... } instead of [ { ... } ][ { ... } { ... } ] (depending on how the exporter is used). Printing only JSON objects is consistent with JSON loggers like zap, for instance.

What do you think about this? I have seen and understood #1819; this is more of a cosmetic change.

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #2196 (1585d76) into main (add511c) will decrease coverage by 0.0%.
The diff coverage is 75.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2196     +/-   ##
=======================================
- Coverage   72.9%   72.9%   -0.1%     
=======================================
  Files        177     176      -1     
  Lines      12161   12161             
=======================================
- Hits        8869    8867      -2     
- Misses      3054    3055      +1     
- Partials     238     239      +1     
Impacted Files Coverage Δ
exporters/stdout/stdouttrace/trace.go 75.5% <75.0%> (-0.1%) ⬇️
...s/otlp/otlptrace/internal/connection/connection.go 14.8% <0.0%> (-1.6%) ⬇️

@MrAlias
Copy link
Contributor

MrAlias commented Aug 24, 2021

This needs a changelog entry describing the change, especially since it is going to change the data format users will be working with.

@luxas
Copy link
Contributor Author

luxas commented Aug 24, 2021

Thanks for the merge of #2195 and reviews 💯!
I rebased this now and added a changelog entry.

@MrAlias MrAlias merged commit c912b17 into open-telemetry:main Aug 25, 2021
@lizthegrey
Copy link
Member

This introduces a race condition because the JSON encoder object is now shared between invocations, and can be potentially called in parallel (trampling its own buffer); I'm opening a PR to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants