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

Allow otlphttpexporter to export in JSON format #6945

Closed
songy23 opened this issue Jan 13, 2023 · 15 comments
Closed

Allow otlphttpexporter to export in JSON format #6945

songy23 opened this issue Jan 13, 2023 · 15 comments
Assignees
Labels
enhancement New feature or request

Comments

@songy23
Copy link
Member

songy23 commented Jan 13, 2023

Is your feature request related to a problem? Please describe.

Currently the otlphttpexporter only supports exporting data in Binary Protobuf Encoding:

req.Header.Set("Content-Type", "application/x-protobuf")

While the otlp spec and the otlp http receiver support both binary protobuf encoding and JSON encoding. It will be nice to be able to export telemetry data in JSON using the otlphttpexporter.

Describe the solution you'd like

Add a config to otlphttpexporter like:

exporters:
  otlphttp:
    ...
    encoding: [binary_protobuf | json]

And have the corresponding encoder in otlphttpexporter.

Feel free to mark this as duplicated if there's an existing issue on otlphttpexporter json support.

@bogdandrutu
Copy link
Member

We want to discourage use of JSON between collectors (unnecessary overhead). We do accept receiving because JS libraries need that.

@vainiusd
Copy link

This was also mentioned in opentelemetry-collector-contrib issue 10836.

It could be useful when sending logs to collectors that do not support OTLP.

@aubm
Copy link

aubm commented Sep 27, 2023

Just to mention that I've been trying to send logs collected by the OpenTelemetry Collector to Logstash and it looks like support for Protobuf is limited and I can't get it working properly.

I've just opened an issue on the Github Logstash Protobuf codec plugin repository.
You can find all the details here: logstash-plugins/logstash-codec-protobuf#70

At this point, if I don't find a way to make it work and I can't get otlphttpexporter to offer JSON encoding, which is well supported by Logstash, I'll have to implement my own exporter.
This is a bummer, and probably an obstacle for more people to adopt it as well.

Just give me the go and I'll submit a PR, it'll be short and sweet.

@atoulme
Copy link
Contributor

atoulme commented Sep 27, 2023

You could try to use the splunkhec exporter in raw mode to push data over to logstash, that's not really meant for it but might get you what you need. If you can push a test showing the problems with logstash, I'm sure folks here can help on their end.

@tvaintrob
Copy link
Contributor

tvaintrob commented Jan 7, 2024

Is there any update regarding this issue?

I'm facing an issue where I need to connect two collector instances over http using json and protobuf is not an option

Adding more context to this:

I am working in a highly regulated environment where security requirements demand payloads being validated deeply by a web application firewall which does not support reading binary data, i will be happy to provide an implementation for this feature request

@aubm
Copy link

aubm commented Jan 8, 2024

Hi @tvaintrob, I ended up starting working on a generic HTTP/JSON exporter.
I might submit a PR to contribute it back eventually if anyone is interested.

The solution suggested by @atoulme to use the splunkhec exporter was interesting, but I couldn't make it work (not sure I remember why exactly, it's been a couple months, but I think it still required me to provide a valid authentication to a splunk server, something like that).

@tvaintrob
Copy link
Contributor

@aubm Thanks for the suggestion! I'll look into that, I also started working on it, I have a fork with the required changes

Hopefully we can push this to the upstream

@songy23 songy23 added discussion-needed Community discussion needed enhancement New feature or request labels Jan 10, 2024
@songy23
Copy link
Member Author

songy23 commented Jan 10, 2024

Looks to me there are enough interests on the otlp http json exporter, I'll bring this up in today's collector SIG

@songy23
Copy link
Member Author

songy23 commented Jan 10, 2024

Recap from today's SIG: exporting OTLP in http/json sounds a valid use case. The spec on SDKs JSON support says:

SDKs SHOULD support both grpc and http/protobuf transports and MUST support at least one of them. If they support only one, it SHOULD be http/protobuf. They also MAY support http/json.

It may not be worth adding a new exporter e.g. otlphttpjsonexporter, but instead can add the JSON encoding to the current otlphttpexporter. The implementation should follow https://opentelemetry.io/docs/specs/otlp/#json-protobuf-encoding, there are a few caveats there.

@aubm @tvaintrob would either of you be interested in taking this issue?

@songy23 songy23 removed the discussion-needed Community discussion needed label Jan 10, 2024
@aubm
Copy link

aubm commented Jan 10, 2024

Thanks @songy23! I would be happy to take the ball. I can work on that this Friday.

@songy23
Copy link
Member Author

songy23 commented Jan 10, 2024

Appreciated @aubm!

@tvaintrob
Copy link
Contributor

Great to hear! @aubm @songy23 i have an implementation ready at https://github.com/tvaintrob/opentelemetry-collector

There is some unrelated stuff like build scripts and workflows, but i can clean it up

@aubm
Copy link

aubm commented Jan 11, 2024

Hey @tvaintrob, if you already did the work, feel free to open a PR, I hadn't started working on it yet myself.
Just let me know if you do plan on submitting it indeed, otherwise I had time scheduled for this tomorrow.

@tvaintrob
Copy link
Contributor

@songy23 @aubm here is the PR, please let me know if any changes are needed

#9276

@songy23 songy23 assigned tvaintrob and unassigned aubm Jan 12, 2024
mx-psi pushed a commit that referenced this issue Feb 7, 2024
…#9276)

**Description:**
This PR adds support for encoding configuration in the `otlphttp`
exporter.

**Link to tracking Issue:** 
#6945

**Testing:** 
Updated existing tests, and added relevant tests 

**Documentation:** 
Updated the `otlphttp` docs to include the new configuration option.

---------

Co-authored-by: Yang Song <songy23@users.noreply.github.com>
@songy23
Copy link
Member Author

songy23 commented Feb 7, 2024

Fixed in #9276, thanks @tvaintrob

@songy23 songy23 closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants