Skip to content

Conversation

@deejgregor
Copy link
Contributor

@deejgregor deejgregor commented Dec 3, 2021

Fixes #202.

Changes

I modeled this change on the Python implementation of ReadableSpan.to_json().

Right now, this only contains support for OStreamSpanExporter, but I'll see if I can add support for the other OStream exporters when I'm on end of year break. Any feedback on my approach in the meantime would be helpful before I do the other exporters. I'll also note that I'm barely familiar with C++, so please don't hesitate to point out even minor issues.

TODO

  • Complete handling of all OwnedAttributeValue variants in PopulateAttribute.

  • Support JSON for OStreamLogExporter and OStreamMetricsExporter.

  • Any other refactoring opportunities?

  • CHANGELOG.md updated for non-trivial changes

  • Unit tests have been added

  • Changes in public API reviewed

@lalitb
Copy link
Member

lalitb commented Dec 3, 2021

Thanks for taking up this item. A couple of points, we can discuss if they don't make sense:

  • As of now, core otel-cpp components ( API, SDK, OStream, and Memory exporters ) don't have any external library dependencies. Do you think we should enable/disable JSON support using macro, as this will not introduce a dependency on nlohmann_json by default?
  • Exporters create their implementation of Recordable to get the data in a format they need. As an example, Zipkin exporter has its own Recordable implementation to format data in JSON format ( https://github.com/open-telemetry/opentelemetry-cpp/blob/main/exporters/zipkin/src/recordable.cc). OStream exporter as of now uses SpanData as default recordable implementation for console output. Do you think we should introduce a new Recordable for JSON?

@esigo
Copy link
Member

esigo commented Dec 7, 2021

Thanks for your contribution.
As OStreamSpanExporter is the entry point for the new users, I believe we should keep it as simple as possible (ideally with no external dependencies and code pollution).
Do you think we can add the JSON support to contrib repo?

@deejgregor
Copy link
Contributor Author

Thanks @lalitb and @esigo. I'll be working on this more over the holidays, and your feedback will be very helpful. Thanks, again!

@lalitb
Copy link
Member

lalitb commented Jan 26, 2022

@deejgregor - Do you have any updates here. Thanks.

@deejgregor deejgregor force-pushed the feature-ostreamexporters-json branch from 621fb05 to 5e55a5b Compare March 8, 2022 21:28
@deejgregor
Copy link
Contributor Author

  • As of now, core otel-cpp components ( API, SDK, OStream, and Memory exporters ) don't have any external library dependencies. Do you think we should enable/disable JSON support using macro, as this will not introduce a dependency on nlohmann_json by default?

Ah, yes. For some reason, I thought nlohmann_json was required by default, and it is not.

+1. That will be a nicer way to go

@lalitb thanks for your comments above, and after (finally) poking around a bit and understanding more, I think it makes sense to do both of these.

@deejgregor
Copy link
Contributor Author

Thanks for your contribution. As OStreamSpanExporter is the entry point for the new users, I believe we should keep it as simple as possible (ideally with no external dependencies and code pollution). Do you think we can add the JSON support to contrib repo?

@esigo I agree about not adding complexity to OStreamSpanExporter and I'll make a new exporter.

What do you think about having the JSON exporter in the main repo, though, instead of contrib as you suggest? On one hand, its only dependency is nlohmann_json which is already used by other exporters in the main repo. On the other hand, since this exporter won't be abled by default since nlohmann_json isn't a base requirement, I'm not sure if this exporter will get much use, so maybe the contrib repo is the better place for it.

Frankly, as I've finally started using a real exporter to Jaeger, my own need for a JSON exporter for testing is pretty much gone. I'm starting to wonder how much use a JSON exporter will be for others.... in particular, is this useful to contribute back at all, regardless of which repo it goes in? Curious on your thoughts (and anyone else's).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add JSON formatting to OStreamExporters

3 participants