fix: Fix OTLP http responses #1010

TylerHelmuth · 2024-02-23T23:47:22Z

Which problem is this PR solving?

Refinery was not returning the correct OTLP payload and not always using the correct Content-Type in the response.

closes Response for OTLP requests does not match OTLP specification #1008

Short description of the changes

This PR attempts to fix the OTLP response while leaving other protocols unaffected.

Creates a new error handler method for the OTLP endpoint
Ensures a ExportTraceServiceResponse object is returned for success and a grpc Status object is used for failure.
Changes the "bad API key" response code from 400 to 401.

MikeGoldsmith

Looks good but added some suggestions around moving some strings to consts

MikeGoldsmith · 2024-02-26T15:38:17Z

route/middleware.go

@@ -58,6 +59,26 @@ func (r *Router) apiKeyChecker(next http.Handler) http.Handler {
 	})
 }

+func (r *Router) apiKeyCheckerOTLP(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		apiKey := req.Header.Get(types.APIKeyHeader)


Might be nice to move API key validation into Husky at some point too

The plan from the API team is to add a public method for that to libhoney, so I'd suggest we wait until they do that.

An update on that: libhoney's planned public method is only to determine a key's classicness, but nothing further in the realm of validity checking.

route/middleware.go

route/otlp_trace.go

TylerHelmuth · 2024-02-26T16:12:24Z

@MikeGoldsmith the apiKeyCheckerOTLP func is a copy of apiKeyChecker to try to keep the existing behavior as close as possible. If we're ok with making changes I'd be in favor of dropping the middleware entirely and handling it all inside postOTLP to centralize the logic.

MikeGoldsmith · 2024-02-26T17:00:12Z

I'm open to making things better instead of just moving stuff around 😄

Leave it better than you found it 🎉

MikeGoldsmith

Looks good 👍🏻

robbkidd · 2024-02-27T21:23:05Z

I'm working on a quick repro to manual test the fix.

TylerHelmuth · 2024-02-27T21:28:00Z

I'm working on a quick repro to manual test the fix.

@robbkidd if it helps, my setup to test was telemetrygen -> collector -> refinery, with the collector exporting otlphttp using.

robbkidd · 2024-02-27T21:37:06Z

I've got otel-cli -> collector 0.95.0 -> refinery.

With a refinery build off the main branch and an invalid API key, I see the expected error in the collector:

Exporting failed. Will retry the request after interval.        {"kind": "exporter", "data_type": "traces", "name": "otlphttp", "error": "error parsing json response: ReadObjectCB: expect { or n, but found \u0000, error found in #0 byte of ...||..., bigger context ...||...", "interval": "7.423832335s"}

With a refinery build off this PR branch and an invalid API key, I don't see any error in the collector when I expect refinery to have sent an error response that collector would parse happily. 😕

TylerHelmuth · 2024-02-27T21:59:35Z

@robbkidd the error you posted is the error this PR solves. It would happen with or without an invalid key.

Are you saying with this PR's build you don't see any error message returned from the call?

robbkidd · 2024-02-27T22:03:25Z

Are you saying with this PR's build you don't see any error message returned from the call?

Correct. With the new build from this PR, I do see an error if I omit the x-honeycomb-team header entirely:

Exporting failed. Dropping data.        {"kind": "exporter", "data_type": "traces", "name": "otlphttp",
 "error": "not retryable error: Permanent error: error exporting items, request to http://refinery:8080/v1/traces responded with HTTP Status Code 401, Message=missing 'x-honeycomb-team' header, Details=[]", "dropped_items": 1}

But I see no error back in the collector when the x-honeycomb-team header is present and given a random key. That should return an Unauthorized error response and fail the collector's export attempt, right?

TylerHelmuth · 2024-02-27T22:07:57Z

@robbkidd I agree with you, I get the same result.

I also tested x-honeycomb-team present with an invalid key against main (using telemetrygen directly) and got no error.

TylerHelmuth · 2024-02-27T22:13:44Z

@robbkidd ok after looking through Refinery this make sense. Refinery is currently only looking for specific keys if AcceptOnlyListedKeys is set, otherwise it just checks that the header is set and has the proper content type. If all that validation passes (which an invalid key in x-honeycomb-team would), Refinery still accepts the span and then, when it tries to use the key to push to Honeycomb, it will fail.

Changing this behavior seems out of scope for this PR. @kentquirk is there history behind this behavior? Should refinery be validating API keys it gets in the request?

kentquirk · 2024-02-27T22:19:25Z

Refinery specifically doesn't validate API keys by default because that then makes rolling keys more difficult. We also have considered (and will again) features where Refinery would maintain its own list of keys and replace some or all of the incoming keys.

If we change anything here, it would be a breaking change, so please don't do that now. We will certainly think about it again but it's a bigger product-level decision.

robbkidd · 2024-02-27T22:47:18Z

@TylerHelmuth Ah! My memory is refreshed! No. Changing that is out of scope. I'll test with a list of acceptable keys for an error direct from Refinery.

robbkidd · 2024-02-27T23:18:09Z

And that worked:

Exporting failed. Dropping data.        {"kind": "exporter", "data_type": "traces", "name": "otlphttp",
 "error": "not retryable error: Permanent error: error exporting items, request to http://refinery:8080/v1/traces responded with HTTP Status Code 401, Message=api key hflurhflauiehg not found in list of authorized keys, Details=[]", "dropped_items": 1}

## Which problem is this PR solving? - Releasing #1010 ## Short description of the changes - update CHANGELOG and RELEASE_NOTES - no documentation updates - no dependency license changes

TylerHelmuth changed the title ~~Start fixing OTLP http responses~~ fix: Fix OTLP http responses Feb 23, 2024

MikeGoldsmith assigned TylerHelmuth Feb 26, 2024

TylerHelmuth marked this pull request as ready for review February 26, 2024 15:26

TylerHelmuth requested a review from a team as a code owner February 26, 2024 15:26

Start fixing OTLP http responses

9262294

TylerHelmuth force-pushed the tyler.fix-otlp-response branch from 3f7dec4 to 9262294 Compare February 26, 2024 15:27

MikeGoldsmith approved these changes Feb 26, 2024

View reviewed changes

Remove unneeded middleware

b032481

MikeGoldsmith approved these changes Feb 27, 2024

View reviewed changes

Merge branch 'main' into tyler.fix-otlp-response

41f5cb9

robbkidd merged commit 6f5aef9 into main Feb 28, 2024
3 checks passed

robbkidd deleted the tyler.fix-otlp-response branch February 28, 2024 18:40

robbkidd mentioned this pull request Feb 28, 2024

rel: prepare v2.4.2 #1017

Merged

robbkidd added a commit that referenced this pull request Feb 28, 2024

rel: prepare v2.4.2 (#1017)

3e69344

## Which problem is this PR solving? - Releasing #1010 ## Short description of the changes - update CHANGELOG and RELEASE_NOTES - no documentation updates - no dependency license changes

robbkidd added type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible. labels Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix OTLP http responses #1010

fix: Fix OTLP http responses #1010

TylerHelmuth commented Feb 23, 2024 •

edited

MikeGoldsmith left a comment

MikeGoldsmith Feb 26, 2024

kentquirk Feb 26, 2024

robbkidd Feb 27, 2024

TylerHelmuth commented Feb 26, 2024

MikeGoldsmith commented Feb 26, 2024

MikeGoldsmith left a comment

robbkidd commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024 •

edited

robbkidd commented Feb 27, 2024 •

edited

TylerHelmuth commented Feb 27, 2024

robbkidd commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024 •

edited

kentquirk commented Feb 27, 2024

robbkidd commented Feb 27, 2024

robbkidd commented Feb 27, 2024 •

edited

fix: Fix OTLP http responses #1010

fix: Fix OTLP http responses #1010

Conversation

TylerHelmuth commented Feb 23, 2024 • edited

Which problem is this PR solving?

Short description of the changes

MikeGoldsmith left a comment

Choose a reason for hiding this comment

MikeGoldsmith Feb 26, 2024

Choose a reason for hiding this comment

kentquirk Feb 26, 2024

Choose a reason for hiding this comment

robbkidd Feb 27, 2024

Choose a reason for hiding this comment

TylerHelmuth commented Feb 26, 2024

MikeGoldsmith commented Feb 26, 2024

MikeGoldsmith left a comment

Choose a reason for hiding this comment

robbkidd commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024 • edited

robbkidd commented Feb 27, 2024 • edited

TylerHelmuth commented Feb 27, 2024

robbkidd commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024

TylerHelmuth commented Feb 27, 2024 • edited

kentquirk commented Feb 27, 2024

robbkidd commented Feb 27, 2024

robbkidd commented Feb 27, 2024 • edited

TylerHelmuth commented Feb 23, 2024 •

edited

TylerHelmuth commented Feb 27, 2024 •

edited

robbkidd commented Feb 27, 2024 •

edited

TylerHelmuth commented Feb 27, 2024 •

edited

robbkidd commented Feb 27, 2024 •

edited