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

httpheader extension: Expose HTTP headers as ClientInfo.Auth #6700

Closed
wants to merge 1 commit into from

Conversation

disq
Copy link

@disq disq commented Dec 10, 2021

Description: New auth extension to expose HTTP headers as ClientInfo.Auth fields, which then can be used in the processor pipeline, as in:

info := client.FromContext(ctx)
// TODO add nil check for info.Auth
val := info.Auth.GetAttribute("X-Forwarded-For")
if str, ok := val.(string); ok {
	fmt.Println("the ip was", val)
}

Documentation:

@disq disq requested review from a team and dmitryax December 10, 2021 19:22
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 10, 2021

CLA Signed

The committers are authorized under a signed CLA.

@jpkrohling
Copy link
Member

This is an interesting approach, but I think it would make more sense to have a new field in the client.Info to hold the connection metadata (or headers, if we want to use that terminology).
https://github.com/open-telemetry/opentelemetry-collector/blob/4dcb3388a1687f10875d41b9757fd80f6b5eb269/client/client.go#L98-L109

Then, the confighttp/configgrpc would populate it, in a similar way it does with the client IP:
https://github.com/open-telemetry/opentelemetry-collector/blob/4dcb3388a1687f10875d41b9757fd80f6b5eb269/config/confighttp/clientinfohandler.go#L39-L51

@jpkrohling jpkrohling assigned jpkrohling and unassigned owais Dec 10, 2021
@disq
Copy link
Author

disq commented Dec 10, 2021

This is an interesting approach, but I think it would make more sense to have a new field in the client.Info to hold the connection metadata (or headers, if we want to use that terminology)

I agree, this is more of a band aid approach while waiting for the collector code to mature (and hopefully we can have these helpers upstream in the future). My use case is a bit different and this was a pain point for us with the http receivers, so wanted to send it in rather than having it sit in a private repo.

@bogdandrutu
Copy link
Member

@disq please provide the motivation for this. @jpkrohling do you know why this is useful?

@disq
Copy link
Author

disq commented Dec 10, 2021

@bogdandrutu we're using OTLP basically as an analytics suite, to collect anonymous usage information (and crash/error reporting to an extent) on a CLI tool. Promscale/Grafana for reporting. This extension enables us to use a "geoip processor" to inject geolocation data into traces as the main collector runs behind a load balancer. I was thinking it might be useful for others.

@jpkrohling
Copy link
Member

do you know why this is useful?

This has been requested a few times in the past, including here: open-telemetry/opentelemetry-collector#2401

@jpkrohling
Copy link
Member

jpkrohling commented Dec 13, 2021

I agree, this is more of a band aid approach while waiting for the collector code to mature

I think we now have everything in place to accommodate this feature. Would you be willing to give it a try?

@disq
Copy link
Author

disq commented Dec 13, 2021

I agree, this is more of a band aid approach while waiting for the collector code to mature

I think we now have everything in place to accommodate this feature. Would you be willing to give it a try?

I'll see what I can do, hopefully this week.

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

Successfully merging this pull request may close these issues.

4 participants