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

feat: Add OTLP handler to send events using OpenTelemetry #290

Merged
merged 19 commits into from Oct 23, 2023

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Oct 19, 2023

Which problem is this PR solving?

Adds option of sending events using OpenTelemetry formatted data (OTLP). Includes a new config option that defaults to libhoney (current export format).

Short description of the changes

  • Add new config optionHANDLER_TYPE that defaults to libhoney
  • Add new otel event handler that uses OTLP to export data, named otel
  • Update k8s metadata lookup to return a map[string]string so it can be used by both handlers
  • Updates existing tests to verify UIDs from k8s resources are strings

How to verify that this has the expected result

No change by default. When setting the new env var to otel, events are sent to Honeycomb using OTLP instead of libhoney.

@MikeGoldsmith MikeGoldsmith changed the title Mike/otel feat: Add OTLP handler to send events using OpenTelemetry Oct 19, 2023
@MikeGoldsmith MikeGoldsmith marked this pull request as ready for review October 19, 2023 15:56
@MikeGoldsmith MikeGoldsmith requested a review from a team as a code owner October 19, 2023 15:56
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm soo excited for this! Amazing work Mike. I left some comments mostly related to keeping telemetry parity with what's currently available with the libhoney event implementation. That said, this works and since it's not yet on by default, I'm somewhat inclined to handle some of the items in the follow-up if they prove to be more time-intensive - especially things that aren't really spec'd out like the headers and newer HTTP attributes.

config/config.go Show resolved Hide resolved
handlers/event_handler.go Show resolved Hide resolved
handlers/otel_handler.go Outdated Show resolved Hide resolved
handlers/otel_handler.go Show resolved Hide resolved
handlers/otel_handler.go Outdated Show resolved Hide resolved
handlers/otel_handler.go Outdated Show resolved Hide resolved
handlers/otel_handler.go Outdated Show resolved Hide resolved
handlers/otel_handler.go Show resolved Hide resolved
@JamieDanielson JamieDanielson added the type: enhancement New feature or request label Oct 19, 2023
@JamieDanielson JamieDanielson added the status: revision needed Waiting for response to changes requested. label Oct 19, 2023
prefix = "http.response.header"
}
attrs := []attribute.KeyValue{}
for key, val := range header {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was working on a test for this and ended up iterating through one more time to get clean strings for header values. I'm not sure if that's right but it did show up more cleanly as attribute values.

Also I adjusted to further match the recommendation of the semantic conventions, which is to lowercase the key and replace hyphens with underscores.

What do you think? 8537b56

Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change needed in setting the response headers, and then a suggestion based on more closely following the semconv recommendation. I included those changes and some adjustments to the smoke test deployment in another branch, let me know what you think!

MikeGoldsmith and others added 2 commits October 23, 2023 13:02
Co-authored-by: Jamie Danielson <jamiedanielson@honeycomb.io>
## Which problem is this PR solving?
More closely follows sem conv when handing headers in the otel handler.

---------

Co-authored-by: JamieDanielson <jamieedanielson@gmail.com>
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woo

@MikeGoldsmith MikeGoldsmith merged commit 34d90a5 into main Oct 23, 2023
3 checks passed
@MikeGoldsmith MikeGoldsmith deleted the mike/otel branch October 23, 2023 15:38
JamieDanielson added a commit that referenced this pull request Oct 23, 2023
…ributes (#296)

## Which problem is this PR solving?
The OTel semantic conventions for headers is to lowercase and replace
`-` with `_` and for each header to be a separate field/attribute. The
logic to do the sanitisation was added in #290 for just the OTel
handler.

This PR moves it to a utility func that both the OTel and libhoney
handlers can use.

## Short description of the changes
- Move sanitisation logic to shared utility func
- Use new func in both otel and libhoney handlers
- Update both handler tests
- Add `-count=1` to test command to clear cache on test runs

## How to verify that this has the expected result
Both libhoney and OTel handlers sanitise headers and add each header as
a separate field / attribute.

---------

Co-authored-by: JamieDanielson <jamieedanielson@gmail.com>
MikeGoldsmith added a commit that referenced this pull request Oct 27, 2023
… HTTP events (#293)

## Which problem is this PR solving?
Updates the OTel handler to try and detect trace context from a HTTP
request's traceparent header if present.

Based on the this PR:
- #290 

## Short description of the changes
- Add new func to otel handler to extract trace context from request
header as use as parent span context if present
- Add unit test to verify extract works
- Update shared handler func that creates test http events to accept
custom header
- Add `Traceparent` to default headers and document in README
- Add `net.component` resource attribute to prevent agent span's from
participating as a full-fledged service within a Honeycomb service map

## How to verify that this has the expected result
Nothing by default. If the otel handler is used and a HTTP request is
captured that contains a trace parent header, the span the agent creates
will use the same trace context

---------

Co-authored-by: Jamie Danielson <jamiedanielson@honeycomb.io>
Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use OTLP instead of Libhoney
2 participants