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

Add an OTLP http/json exporter #3651

Closed
carlosalberto opened this issue Sep 22, 2021 · 14 comments
Closed

Add an OTLP http/json exporter #3651

carlosalberto opened this issue Sep 22, 2021 · 14 comments
Assignees

Comments

@carlosalberto
Copy link
Contributor

Based on open-telemetry/opentelemetry-specification#1957 we would like to add prototypes in other languages to validate this.

Lightstep has interest in this, so if everybody has interest in this (or has his/her own prototype) please speak up ;)

@tnevolin
Copy link
Contributor

Here I am.

@jkwatson
Copy link
Contributor

jkwatson commented Sep 22, 2021

@anuraaga and I had decided that we don't particular think this is of high value when we already have OTLP/grpc and OTLP/http. What is the benefit of having yet another OTLP http-based exporter?

@carlosalberto
Copy link
Contributor Author

Overall we need prototypes in other languages - @jkwatson do you think then this would be a bad idea to add it, even if Lightstep were to contribute it fully?

@jkwatson
Copy link
Contributor

Overall we need prototypes in other languages - @jkwatson do you think then this would be a bad idea to add it, even if Lightstep were to contribute it fully?

more code to maintain. Could it be built in opentelemetry-java-contrib for the prototype?

@anuraaga
Copy link
Contributor

I think it actually is a bad idea to be honest, even in the contrib repo. It's yet another option users need to choose from - while grpc and http do have clear tradeoffs and use cases (http2 vs 1, large binary vs small, etc), json doesn't - it's actually worse in basically every way I think, less performance, larger binary size due to inclusion of jackson. I think there's only potential for user confusion with no benefit if we add the JSON export.

I understand the desire for a prototype but it still should be something people actually use I guess to be a valid prototype.

Shouldn't one prototype in JS (I think the main use case) be enough given it'll either work or not work?

@anuraaga
Copy link
Contributor

So we do use json for the logging OTLP exporter - perhaps that can be used in some way for this. If collector had a receiver that accepted log lines of ResourceSpans json that would be useful!

@carlosalberto
Copy link
Contributor Author

@anuraaga In JS it already exists, but the point is that we need to test in other languages (if the time is needed). Having it in a single language is not much of help.

So we do use json for the logging OTLP exporter - perhaps that can be used in some way for this.

That sounds like a hack and I wouldn't go for it.

As the name implies, it's only a prototype - we may even leave it as as something like EXPERIMENTAL, and retire it or something (once we validated a few things, so we know that if we ever need this in any language other than JS, we are good to go). Also, being in the contrib repo means you don't have to maintain it.

Let me know ;)

@anuraaga
Copy link
Contributor

That sounds like a hack and I wouldn't go for it

An http/json exporter seems like more of a hack really - the log exporter is actually useful.

we may even leave it as as something like EXPERIMENTAL, and retire it or something

That sounds like it doesn't need to even be merged or could live in a fork right? I don't want users to find this.

@anuraaga
Copy link
Contributor

FWIW we've gone through a lot of effort making the proto exporter very efficient with no binary size or similar tradeoff at all. Languages that haven't done this would probably be more receptive to trying a json approach, we just don't really have any use case for it.

@carlosalberto
Copy link
Contributor Author

carlosalberto commented Sep 23, 2021

An http/json exporter seems like more of a hack really

Go tell that to the JS group ;)

But jokes aside, as I said, there's no actual use of it right now in Java, other than verifying the encoding and the rest will not be a problem in the future for other languages (better be safe than sorry). So yes, let's keep it in another repo, open a Draft PR, keep it there for discussing it, and that would be all.

@anuraaga
Copy link
Contributor

I have a feeling they'd mostly agree :)

Draft PR or similar seems good. It should be pretty small, we already have the marshaling

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/otlp/internal/traces/TraceRequestMarshalerTest.java#L364

Since we don't have to worry about making it publishable it might just be two lines of change in this file

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/ProtoRequestBody.java#L44

@tnevolin
Copy link
Contributor

tnevolin commented Sep 24, 2021

@anuraaga
I see you proposed code examples to modify in main SDK repository.
Do you want modification and draft PR to be done against this repository or move exporter completely to contrib one and make draft PR from there?

At least we can agree on what should be done in main repository and then I'll move the code to contribution.

@jkwatson
Copy link
Contributor

Draft PR here is fine for spec review...we won't plan on actually merging it.

@jack-berg
Copy link
Member

This has been added for testing purposes in #4435.

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

No branches or pull requests

5 participants