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

otlp receiver expects traces on “/v1/trace” but spec says “/v1/traces”. #1968

Closed
tigrannajaryan opened this issue Oct 16, 2020 · 4 comments · Fixed by #1979
Closed
Assignees
Labels
bug Something isn't working

Comments

@tigrannajaryan
Copy link
Member

Probably need to fix this gracefully by adding “/v1/traces” as an alias, then removing “/v1/trace” in a few months after announcement.

@tigrannajaryan tigrannajaryan added the bug Something isn't working label Oct 16, 2020
@tigrannajaryan tigrannajaryan self-assigned this Oct 16, 2020
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Oct 18, 2020
@bogdandrutu
Copy link
Member

bogdandrutu commented Oct 18, 2020

I think this gets very ugly:

  • gRPC traffic uses v1/trace path
  • HTTP traffic uses v1/traces path

Probably it is good to proceed and change both gRPC, HTTP to use v1/traces.

Here is a possible plan to do this in a backwards compatible way:

  1. Change protos to use the new path with v1/traces, but also keep the gRPC service definition that we currently have (will use request and response objects from the new path).
  2. Change receivers to install both, this will make the receiver be able to receive on v1/trace and v1/traces both traffic gRPC and HTTP. This is backwards compatible.
  3. Change the otlp (gRPC) exporter to:
    • First try to send to the new service path. If status returned is "Unimplemented" fallback to the old path.
    • In few months remove the fallback path.
  4. Change the otlphttp not necessary.

tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change patches
the gRPC gateway path pattern to match what spec requires.

Fixes: open-telemetry#1968
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change patches
the gRPC gateway path pattern to match what spec requires.

Fixes: open-telemetry#1968
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change patches
the gRPC gateway path pattern to match what spec requires.

Fixes: open-telemetry#1968
@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Oct 19, 2020

@bogdandrutu I was initially thinking to keep gRPC unchanged for 2 reasons:

  1. The OTLP spec does not say anything explicitly about OTLP/gRPC path. It does explicitly say what the path is for OTLP/HTTP.

  2. OTLP/gRPC does not use /v1/trace, it uses /opentelemetry.proto.collector.trace.v1.TraceService/Export as the path. So, although it would be nice to have OTLP/gRPC path also use singular "trace" in it, I don't think it is a big deal. It is a different path compared to OTLP/HTTP anyway and whoever needs to deal directly with the paths is not going to be able to use one value for both OTLP/HTTP and OTLP/gRPC protocols (for example in a hypothetical scenario when they need to map the paths in the proxy to something else).

Because of the above 2 reasons I suggest that we don't change anything in OTLP/gRPC and keep it as is. We only fix the path in OTLP/HTTP. Here I was also initially thinking about a graceful, so that both paths are supported by receivers for a while, but now I think that even that is not necessary. OTLP/HTTP implementation was added to Collector just a few days ago, it is not even released. It is very likely that nobody in the wild is using this protocol yet so it should be OK to fix the bug without attempting to maintain compatibility with previous incorrect version of OTLP receiver.

What do you think?

@tigrannajaryan
Copy link
Member Author

I can look into implementing the fix for OTLP/HTTP in a graceful way, and keep accepting the old path in the receiver for a while, but it may complicate the fix.

tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: open-telemetry#1968
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: open-telemetry#1968
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: open-telemetry#1968
tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: open-telemetry#1968
@tigrannajaryan
Copy link
Member Author

Fix for OTLP/HTTP only with graceful handling of both old and new path in OTLP receiver: #1979

tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: open-telemetry#1968
bogdandrutu pushed a commit that referenced this issue Oct 19, 2020
OTLP/HTTP receiver was incorrectly expecting traces on /v1/trace.
OTLP spec require the path to be /v1/traces. This change adds /v1/traces
as a supported alias in the receiver to match what spec requires.

We will keep both paths supported for a while and then will eventually
retire /v1/trace.

Note that the OTLP/HTTP exporter already correctly uses /v1/traces path.

Fixes: #1968
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Oct 19, 2020
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Oct 20, 2020
bogdandrutu added a commit that referenced this issue Oct 20, 2020
* Add and enable loadtest for OTLP/HTTP

Signed-off-by: Bogdan Drutu <[email protected]>

* Fix path usage for otlphttp

Signed-off-by: Bogdan Drutu <[email protected]>

* Fix paths to handle wrong path for trace

Signed-off-by: Bogdan Drutu <[email protected]>

* Increase RAM usage for otlphttp trace test

Signed-off-by: Bogdan Drutu <[email protected]>

* Disable trace loadtests until #1968 gets resolved

Signed-off-by: Bogdan Drutu <[email protected]>

* Enable back OTLP-HTTP for trace

Signed-off-by: Bogdan Drutu <[email protected]>

* Fix review comments

Signed-off-by: Bogdan Drutu <[email protected]>

* Add changelog entry

Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Jan 20, 2022
This was changed 1y ago, and it is probably safe to remove it now.

Updates open-telemetry#1968

Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Jan 20, 2022
This was changed 1y ago, and it is probably safe to remove it now.

Updates open-telemetry#1968

Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu added a commit that referenced this issue Jan 21, 2022
This was changed 1y ago, and it is probably safe to remove it now.

Updates #1968

Signed-off-by: Bogdan Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants