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

Open Telemetry Console Exporter Json output #5588

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

meijeran
Copy link

@meijeran meijeran commented May 2, 2024

Fixes #5036
Design discussion issue #

Changes

Please provide a brief description of the changes here.

This is an attempt to solve #5036, I switched the current output to a json variant.
I am not sure if we want to keep the original output as well and add the json variant as an extra option. According the comments in #4548 only one output format is highly favored.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@CodeBlanch
Copy link
Member

I definitely think some options/levers are going to be needed.

  • The primary use case today for ConsoleExporter is dev/debug/test. The main thing we need for this is readability. I don't think JSON is too bad but some have suggested YAML might be better. Or a very simple text format. We could have options to pick one with the most simple/easy to read as the default. We don't have to start with the options or multiple formats those could be added later but I would be interested to see a comparison for the change her vs what we have today.

  • The use case users seem to be asking for on the issue is for production output to use with scraping of stdout. This is a very different thing. I think for this we need the JSON to be compact (single-line per entry). Also it should probably use JSON Protobuf Encoding (example: trace.json). We probably also need to improve the performance to make it production ready but that doesn't need to happen on this PR. Do we even want the ConsoleExporter to be this thing? Or perhaps could we have a StandardOutputExporter in contrib or something which is purpose built for this scenario? We could document ConsoleExporter is only supported for dev/debug/test and link to the other thing for users who want to scrape. Just an idea 🤔

So my question is really which scenario is this for? Does this really fix/resolve the issue?

@cijothomas
Copy link
Member

cijothomas commented May 2, 2024

ConsoleExporter is explicitly not recommended for production scenarios. There is no spec written for ensuring consistent output. And yes, performance is another concern.

For scraping the stdout, a better option is to use collector, and have it output to stdout.
Edit : collectors output is also not fixed format.

@cijothomas
Copy link
Member

Can you share couple of screenshots of the new behavior as well?

@meijeran
Copy link
Author

meijeran commented May 2, 2024

So currently it looks like this

image

image

@reyang
Copy link
Member

reyang commented May 2, 2024

Is this JSON format the same as the proto JSON format or a custom one? https://opentelemetry.io/docs/specs/otlp/#json-protobuf-encoding

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 73.68421% with 15 lines in your changes missing coverage. Please review.

Project coverage is 85.85%. Comparing base (6250307) to head (f5bd25b).
Report is 257 commits behind head on main.

Current head f5bd25b differs from pull request most recent head 21f391f

Please upload reports for the commit 21f391f to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5588      +/-   ##
==========================================
+ Coverage   83.38%   85.85%   +2.47%     
==========================================
  Files         297      265      -32     
  Lines       12531    11321    -1210     
==========================================
- Hits        10449     9720     -729     
+ Misses       2082     1601     -481     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 85.86% <73.68%> (?)
unittests-Solution-Stable 85.69% <73.68%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...emetry.Exporter.Console/ConsoleActivityExporter.cs 75.75% <73.68%> (+27.48%) ⬆️

... and 112 files with indirect coverage changes

@meijeran
Copy link
Author

meijeran commented May 3, 2024

It is not the proto json format is it just a serialized Activity

@meijeran meijeran marked this pull request as ready for review May 7, 2024 09:15
@meijeran meijeran requested a review from a team as a code owner May 7, 2024 09:15
@cijothomas
Copy link
Member

I definitely think some options/levers are going to be needed.

  • The primary use case today for ConsoleExporter is dev/debug/test. The main thing we need for this is readability. I don't think JSON is too bad but some have suggested YAML might be better. Or a very simple text format. We could have options to pick one with the most simple/easy to read as the default. We don't have to start with the options or multiple formats those could be added later but I would be interested to see a comparison for the change her vs what we have today.
  • The use case users seem to be asking for on the issue is for production output to use with scraping of stdout. This is a very different thing. I think for this we need the JSON to be compact (single-line per entry). Also it should probably use JSON Protobuf Encoding (example: trace.json). We probably also need to improve the performance to make it production ready but that doesn't need to happen on this PR. Do we even want the ConsoleExporter to be this thing? Or perhaps could we have a StandardOutputExporter in contrib or something which is purpose built for this scenario? We could document ConsoleExporter is only supported for dev/debug/test and link to the other thing for users who want to scrape. Just an idea 🤔

So my question is really which scenario is this for? Does this really fix/resolve the issue?

@meijeran Could you reply to the above comment? We are trying to see the issues attempted to be resolved with this PR, so as to better review/guide!

@meijeran
Copy link
Author

@cijothomas Great questions!

The main reason I started this change is because of issue #5036 and some comments from this discussion. There were a few requests to standardize the console output, so this proposal is just an idea to see if we can make that work.

If the goal is to scrape this data, I think having a dedicated exporter in contrib would be the way to go instead of relying on this. But I'm also curious why the other exporter options aren't enough for that.

So, to answer your question, I'm not entirely sure if this fixes the issue completely, but it's something people have been asking for. Would love to hear more thoughts on this!

Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added Stale and removed Stale labels May 30, 2024
@meijeran
Copy link
Author

meijeran commented Jun 4, 2024

Is it still worth to continue on this? Happy to help or figuring out to way to make this work

@reyang
Copy link
Member

reyang commented Jun 4, 2024

Is it still worth to continue on this? Happy to help or figuring out to way to make this work

I think yes, definitely something we want to improve.

My suggestion: @meijeran would you clarify the goal here? I think it will be either 1) improving the readability of the stdout exporter or 2) supporting OTLP JSON file (including stdout) exporter for better interoperability.

Once the goal is clear, depending on which one you want to tackle, I have different suggestions:

  • If the goal is 1) improving the readability of the stdout exporter, I would suggest YAML rather than JSON
  • If the goal is 2) supporting OTLP JSON file/stdout exporter, I would suggest doing it in another exporter rather than the OpenTelemetry.Exporter.Console

@mrmartan
Copy link

mrmartan commented Jun 7, 2024

Goal 2) for us.

The use case is: we are running apps on k8s. They are logging single line json (the standard dotnet log format/content) to stdout that is being tailed and shipped by promtail/grafana-agent/alloy. We now want to change the format to OTEL compliant logs without changing the sipping mechanism, i.e. keep the k8s stdout approach.

@Kielek Kielek added the pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package label Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I would like to configure Open Telemetry Console Exporter to write using Json Format
6 participants