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

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

enuret
Copy link

@enuret enuret commented Dec 22, 2023

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.

@pellared pellared changed the title Add onfiguration option to override Host header. Add configuration option to override Host header Dec 22, 2023
@dmathieu
Copy link
Member

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

@enuret
Copy link
Author

enuret commented Dec 23, 2023

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
Copy link
Member

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
Copy link
Author

enuret commented Jan 2, 2024

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

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

dmathieu commented Jan 2, 2024

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
Copy link
Author

enuret commented Jan 2, 2024

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 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
Copy link
Author

enuret commented Jan 4, 2024

@dmathieu I reverted to the WithHostHeader version

Copy link

codecov bot commented Jan 4, 2024

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

Impacted file tree graph

@@          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
Copy link
Author

enuret commented Jan 4, 2024

@dmathieu fixed Lint + rebase

@dmathieu
Copy link
Member

dmathieu commented Jan 8, 2024

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

@enuret
Copy link
Author

enuret commented Jan 8, 2024

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
Copy link
Member

dmathieu commented Jan 8, 2024

make lint, same as the CI itself runs.

@enuret
Copy link
Author

enuret commented Jan 12, 2024

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
Copy link
Author

enuret commented Jan 12, 2024

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

  1. make check-clean-work-tree
    doesn't give me any errors, while it gives some errors in the CI

  2. 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
  1. 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
Copy link
Author

enuret commented Jan 12, 2024

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

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

enuret commented Jan 29, 2024

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
Copy link
Member

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
Copy link
Contributor

MadVikingGod commented Feb 1, 2024

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.

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

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?

Comment on lines +158 to +160
// config.WithEndpoint("myenvoyhost")
//
// .WithHostHeader("mytargetotelcollectorhost")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +103 to +104
// WithHostHeader allows one to tell the driver to override HTTP host header.
// If value is unset Endpoint's host is used as Host header.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

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

@pellared
Copy link
Member

pellared commented Mar 12, 2024

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

@pellared
Copy link
Member

pellared commented Apr 9, 2024

@enuret, bump.

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.

7 participants