Add `WithHostHeader` config option to override Host header in opentelemetry requests #4780

enuret · 2023-12-22T15:18:04Z

Adding configuration option to override Host header. This header is not overriding by providing it as part of overrided Headers and a separate field "Host" in Request is used.
https://pkg.go.dev/net/http#Request

Host header is used by some load balancers, for example, envoy uses it for routing requests.
The similar change is planned to be made in otelcollector

Use case:
My go client sends otel data via envoy
go client -> envoy -> otel collector

Since envoy performs routing is based on "Host" header , it has to be added into request.

dmathieu · 2023-12-23T09:21:29Z

Why can't you use the "WithHeaders" config?

enuret · 2023-12-23T11:23:37Z

Why can't you use the "WithHeaders" config?

Opentelemetry client uses net.http.Request to make http requests and this library doesn't override Host header when it is passed as Request.Headers field and that's why WithHeaders doesn't work for Host header. They have a special field Request.Host if Host header has to be overrided.

This is a reason why I added a separate configuration for Host to be consistent with the net.http go library. I worked with Java instrumentation and there is no such behavior, Headers overrides host as well.

I can alternatively check if Host is in the headers when forming the client request and then set up this Request.Host field instead of adding a separate configuration if it looks like a more preferred way.

dmathieu · 2023-12-23T23:39:11Z

Thank you for the details.
I think exposing this option, when we already allow setting a host when the endpoint is initialized is exposing a net/http implementation detail.

You could use a different Host than the one set when we create the http request with a custom transport which would modify the request accordingly and call the proper parent transport to make the request accordingly.

enuret · 2024-01-02T11:18:47Z

You could use a different Host than the one set when we create the http request with a custom transport which would modify the request accordingly and call the proper parent transport to make the request accordingly.

Hi, @dmathieu I didn't understand where you meant calling parent transport protocol, can you please clarify what you meant by that? In the meantime I changed preparation request place to check header name and set it to request where we copy headers. Please check if it is ok

dmathieu · 2024-01-02T14:40:51Z

My apologies. I thought the otlp exporter allowed specifying a custom RoundTripper, which would have been more extensible, as it would have allowed you to modify anything within the request.
We don't provide that option, and previous discussions about it have other issues (see #1858).

Between both of your solutions, however, I think an option is better than reusing the Headers one, which is a bit too hacky.

enuret · 2024-01-02T16:07:47Z

Between both of your solutions, however, I think an option is better than reusing the Headers one, which is a bit too hacky.

I agree, let me revert my last commit

enuret · 2024-01-04T10:28:25Z

@dmathieu I reverted to the WithHostHeader version

codecov · 2024-01-04T15:16:45Z

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 83.4%. Comparing base (76921e9) to head (563d2f3).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #4780   +/-   ##
=====================================
  Coverage   83.4%   83.4%           
=====================================
  Files        238     238           
  Lines      15745   15779   +34     
=====================================
+ Hits       13143   13174   +31     
- Misses      2314    2317    +3     
  Partials     288     288

Files	Coverage Δ
...tlpmetric/otlpmetricgrpc/internal/oconf/options.go	`89.5% <100.0%> (+0.3%)`	⬆️
exporters/otlp/otlpmetric/otlpmetrichttp/client.go	`82.3% <100.0%> (+0.2%)`	⬆️
exporters/otlp/otlpmetric/otlpmetrichttp/config.go	`87.0% <100.0%> (+0.8%)`	⬆️
...tlpmetric/otlpmetrichttp/internal/oconf/options.go	`92.8% <100.0%> (+0.2%)`	⬆️
...pmetric/otlpmetrichttp/internal/otest/collector.go	`61.1% <100.0%> (+0.5%)`	⬆️
...trace/otlptracegrpc/internal/otlpconfig/options.go	`93.6% <100.0%> (+0.2%)`	⬆️
exporters/otlp/otlptrace/otlptracehttp/client.go	`79.2% <100.0%> (+0.2%)`	⬆️
...trace/otlptracehttp/internal/otlpconfig/options.go	`92.1% <100.0%> (+0.2%)`	⬆️
exporters/otlp/otlptrace/otlptracehttp/options.go	`100.0% <100.0%> (ø)`
...pmetric/otlpmetricgrpc/internal/otest/collector.go	`22.3% <0.0%> (-0.4%)`	⬇️

enuret · 2024-01-04T15:43:06Z

@dmathieu fixed Lint + rebase

CHANGELOG.md

exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go

internal/shared/otlp/otlpmetric/otest/collector.go.tmpl

internal/shared/otlp/otlptrace/otlpconfig/options.go.tmpl

internal/shared/otlp/otlpmetric/oconf/options.go.tmpl

dmathieu · 2024-01-08T15:09:36Z

You can run lints on your own machine. You do not need to rely on GH CI to check them for you.

enuret · 2024-01-08T15:59:53Z

You can run lints on your own machine. You do not need to rely on GH CI to check them for you.

which command should I use to run locally? I tried but it gives ablotulely different result from what I see in CI, I run
golangci-lint run

dmathieu · 2024-01-08T16:04:48Z

make lint, same as the CI itself runs.

exporters/otlp/otlpmetric/otlpmetrichttp/config.go

enuret · 2024-01-12T13:43:27Z

make lint, same as the CI itself runs.

I actually cannot make it work

Updating intra-repository dependencies in all go modules
go mod tidy in .
go mod tidy in ./bridge/opencensus
go: downloading github.com/kr/text v0.1.0
go mod tidy in ./bridge/opencensus/test
go mod tidy in ./bridge/opentracing
go mod tidy in ./bridge/opentracing/test
go mod tidy in ./example/dice
go mod tidy in ./example/namedtracer
go mod tidy in ./example/opencensus
go mod tidy in ./example/otel-collector
go mod tidy in ./example/passthrough
go mod tidy in ./example/prometheus
go mod tidy in ./example/zipkin
go mod tidy in ./exporters/otlp/otlpmetric/otlpmetricgrpc
go mod tidy in ./exporters/otlp/otlpmetric/otlpmetrichttp
go mod tidy in ./exporters/otlp/otlptrace
go mod tidy in ./exporters/otlp/otlptrace/otlptracegrpc
go mod tidy in ./exporters/otlp/otlptrace/otlptracehttp
go mod tidy in ./exporters/prometheus
go mod tidy in ./exporters/stdout/stdoutmetric
go mod tidy in ./exporters/stdout/stdouttrace
go mod tidy in ./exporters/zipkin
go mod tidy in ./internal/tools
go: downloading github.com/cloudflare/circl v1.3.7
go mod tidy in ./metric
go mod tidy in ./schema
go mod tidy in ./sdk
go mod tidy in ./sdk/metric
go mod tidy in ./trace
golangci-lint .
golangci-lint ./bridge/opencensus
golangci-lint ./bridge/opencensus/test
golangci-lint ./bridge/opentracing
golangci-lint ./bridge/opentracing/test
ERRO [linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies 
make: *** [golangci-lint/./bridge/opentracing/test] Error 7```

enuret · 2024-01-12T13:49:10Z

I have a bunch of things which probably work not in the way they are supposed to

make check-clean-work-tree
doesn't give me any errors, while it gives some errors in the CI
when I do make precommit
it changes one line in many files

-package oconf // import "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf"
+package oconf

make has some errors

make
go generate ./...
go generate ./bridge/opencensus/...
go generate ./bridge/opencensus/test/...
go generate ./bridge/opentracing/...
go generate ./bridge/opentracing/test/...
go generate ./example/dice/...
go generate ./example/namedtracer/...
go generate ./example/opencensus/...
go generate ./example/otel-collector/...
go generate ./example/passthrough/...
go generate ./example/prometheus/...
go generate ./example/zipkin/...
go generate ./exporters/otlp/otlpmetric/otlpmetricgrpc/...
go generate ./exporters/otlp/otlpmetric/otlpmetrichttp/...
go generate ./exporters/otlp/otlptrace/...
go generate ./exporters/otlp/otlptrace/otlptracegrpc/...
go generate ./exporters/otlp/otlptrace/otlptracehttp/...
go generate ./exporters/prometheus/...
go generate ./exporters/stdout/stdoutmetric/...
go generate ./exporters/stdout/stdouttrace/...
go generate ./exporters/zipkin/...
go generate ./metric/...
go generate ./schema/...
go generate ./sdk/...
go generate ./sdk/metric/...
stringer: no values defined for type InstrumentKind
instrument.go:15: running "stringer": exit status 1
stringer: no values defined for type Temporality
metricdata/temporality.go:15: running "stringer": exit status 1
make: *** [go-generate/./sdk/metric] Error 1

I don't where the problem is, maybe versions of something do not match..
And that's why I rely on CI results, but now I am stuck cause don't know why make check-clean-work-tree fails in CI but doesn't fail locally

enuret · 2024-01-12T15:24:52Z

Ok, found that I used go 1.21, and on 1.20 it is fine

MrAlias

I do not think we should add another option when we already have WithHeaders. This will only add confusion (how are conflicts resolved; which is the recommended option to use?).

Why not parse the headers passed to WithHeaders and set the Host field if it is passed there?

enuret · 2024-01-29T19:14:24Z

I do not think we should add another option when we already have WithHeaders. This will only add confusion (how are conflicts resolved; which is the recommended option to use?).

Why not parse the headers passed to WithHeaders and set the Host field if it is passed there?

I actually agree with you now, it will be consistent with how i did open-telemetry/opentelemetry-collector#9411
and more predictable on user side.

dmathieu · 2024-01-29T21:03:25Z

I suggested not using WithHeaders to better match the http.Request API, which has its own attribute and doesn't automagically retrieve the host from there.

MadVikingGod · 2024-02-01T15:59:32Z

I do not think we should add another option when we already have WithHeaders. This will only add confusion (how are conflicts resolved; which is the recommended option to use?).

Why not parse the headers passed to WithHeaders and set the Host field if it is passed there?

I think we should add ~~WithHeaders~~ WithHostHeader .

I don't think there is any confusion if you already use the http package. http will strip out any Host header you put in your request.Headers, and will only send Host if you use request.Host. We will follow the same logic, just with options instead of fields on the request. There also doesn't need to be any conflict resolution; if you include Host in your headers, the underlying http.Client will ignore it, and we will take no action to fill it in.

I made a gist to demonstrate the behavior: https://gist.github.com/MadVikingGod/16a92daebf61c67a7ff7d846b11d93c4
It's not a playground link because httptest hangs on the playground.

MadVikingGod

This looks like it only affects HTTP. I'm ok with where it is.

If we were to use this kind of option for GRPC, a space which I'm less familiar with, would this be exposed in a similar way, or would we take a different approach?

MrAlias · 2024-02-05T15:31:23Z

exporters/otlp/otlpmetric/otlpmetrichttp/config.go

+// config.WithEndpoint("myenvoyhost")
+//
+//	.WithHostHeader("mytargetotelcollectorhost")


This doesn't look correctly formatted to be rendered as code nor does it look valid. WithHostHeader is an argument passed to a config new function, this makes it look like a method being called.

MrAlias · 2024-02-05T15:32:44Z

exporters/otlp/otlptrace/otlptracehttp/options.go

+// WithHostHeader allows one to tell the driver to override HTTP host header.
+// If value is unset Endpoint's host is used as Host header.


This comment differs from the metric one. They should be unified.

XSAM

Overall, It looks good, but the comments @MrAlias mentioned should be resolved.

pellared · 2024-03-12T09:00:45Z

@enuret Can you please address @MrAlias comments and fix the build?

pellared · 2024-04-09T05:54:45Z

@enuret, bump.

Add onfiguration option to override Host header.

99f34b3

enuret requested review from MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared, hanyuancheung and dmathieu as code owners December 22, 2023 15:18

pellared changed the title ~~Add onfiguration option to override Host header.~~ Add configuration option to override Host header Dec 22, 2023

enuret force-pushed the otel-host-header branch from 5f00ebe to 16a4d10 Compare January 2, 2024 11:07

enuret changed the title ~~Add configuration option to override Host header~~ Pass custom host header when it is set in WithHeaders() Jan 2, 2024

enuret force-pushed the otel-host-header branch from bba2573 to 99f34b3 Compare January 2, 2024 16:11

enuret changed the title ~~Pass custom host header when it is set in WithHeaders()~~ Add WithHostHeader config option to override Host header in opentelemetry requests Jan 4, 2024

enuret and others added 3 commits January 4, 2024 16:34

Fix linter

ccbd51b

Merge branch 'main' into otel-host-header

d182c08

Fix linter

d4f0088

enuret and others added 2 commits January 8, 2024 13:10

Merge branch 'main' into otel-host-header

ff53f58

fix linter: make precommit

f802723

dmathieu reviewed Jan 8, 2024

View reviewed changes

enuret added 3 commits January 8, 2024 15:24

shared file for lint

0c612a8

Lint for config.go

ece66a6

run precommit

2fbec02

Merge branch 'main' into otel-host-header

e9f2027

dmathieu reviewed Jan 9, 2024

View reviewed changes

exporters/otlp/otlpmetric/otlpmetrichttp/config.go Show resolved Hide resolved

Merge branch 'main' into otel-host-header

68d6185

Add comment & fix lint

e18a523

enuret force-pushed the otel-host-header branch from c622bb7 to e18a523 Compare January 12, 2024 15:41

enuret added 3 commits January 12, 2024 16:51

Merge branch 'main' into otel-host-header

0e233cc

Merge branch 'main' into otel-host-header

31e41ed

Merge branch 'main' into otel-host-header

ea1dd77

enuret mentioned this pull request Jan 25, 2024

Add support of Host header in otlphttpexporter open-telemetry/opentelemetry-collector#9395

Closed

MrAlias reviewed Jan 29, 2024

View reviewed changes

MadVikingGod approved these changes Feb 5, 2024

View reviewed changes

MrAlias reviewed Feb 5, 2024

View reviewed changes

XSAM approved these changes Feb 15, 2024

View reviewed changes

hanyuancheung approved these changes Feb 26, 2024

View reviewed changes

enuret and others added 2 commits February 26, 2024 13:41

Merge branch 'main' into otel-host-header

f1fef37

Merge branch 'main' into otel-host-header

563d2f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `WithHostHeader` config option to override Host header in opentelemetry requests #4780

Add `WithHostHeader` config option to override Host header in opentelemetry requests #4780

enuret commented Dec 22, 2023

dmathieu commented Dec 23, 2023

enuret commented Dec 23, 2023

dmathieu commented Dec 23, 2023

enuret commented Jan 2, 2024 •

edited

dmathieu commented Jan 2, 2024

enuret commented Jan 2, 2024 •

edited

enuret commented Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

enuret commented Jan 4, 2024

dmathieu commented Jan 8, 2024

enuret commented Jan 8, 2024

dmathieu commented Jan 8, 2024

enuret commented Jan 12, 2024

enuret commented Jan 12, 2024

enuret commented Jan 12, 2024

MrAlias left a comment

enuret commented Jan 29, 2024 •

edited

dmathieu commented Jan 29, 2024

MadVikingGod commented Feb 1, 2024 •

edited

MadVikingGod left a comment

MrAlias Feb 5, 2024

MrAlias Feb 5, 2024

XSAM left a comment

pellared commented Mar 12, 2024 •

edited

pellared commented Apr 9, 2024

		// WithHostHeader allows one to tell the driver to override HTTP host header.
		// If value is unset Endpoint's host is used as Host header.

Add WithHostHeader config option to override Host header in opentelemetry requests #4780

Are you sure you want to change the base?

Add WithHostHeader config option to override Host header in opentelemetry requests #4780

Conversation

enuret commented Dec 22, 2023

dmathieu commented Dec 23, 2023

enuret commented Dec 23, 2023

dmathieu commented Dec 23, 2023

enuret commented Jan 2, 2024 • edited

dmathieu commented Jan 2, 2024

enuret commented Jan 2, 2024 • edited

enuret commented Jan 4, 2024

codecov bot commented Jan 4, 2024 • edited

Codecov Report

enuret commented Jan 4, 2024

dmathieu commented Jan 8, 2024

enuret commented Jan 8, 2024

dmathieu commented Jan 8, 2024

enuret commented Jan 12, 2024

enuret commented Jan 12, 2024

enuret commented Jan 12, 2024

MrAlias left a comment

Choose a reason for hiding this comment

enuret commented Jan 29, 2024 • edited

dmathieu commented Jan 29, 2024

MadVikingGod commented Feb 1, 2024 • edited

MadVikingGod left a comment

Choose a reason for hiding this comment

MrAlias Feb 5, 2024

Choose a reason for hiding this comment

MrAlias Feb 5, 2024

Choose a reason for hiding this comment

XSAM left a comment

Choose a reason for hiding this comment

pellared commented Mar 12, 2024 • edited

pellared commented Apr 9, 2024

Add `WithHostHeader` config option to override Host header in opentelemetry requests #4780

Add `WithHostHeader` config option to override Host header in opentelemetry requests #4780

enuret commented Jan 2, 2024 •

edited

enuret commented Jan 2, 2024 •

edited

codecov bot commented Jan 4, 2024 •

edited

enuret commented Jan 29, 2024 •

edited

MadVikingGod commented Feb 1, 2024 •

edited

pellared commented Mar 12, 2024 •

edited