feat(opentelemetry source): add support for enriching logs with HTTP headers #21674

jblazquez · 2024-11-01T01:29:24Z

Summary

This PR adds a new http.headers configuration to the opentelemetry source, which injects HTTP headers received in the OTLP/HTTP request into the log events, similarly to how http_server does it. Please see #21673 for the motivation behind this change.

This is my first contribution to Vector, so I'm not sure whether the changes in this PR are in the right form to be accepted. I wanted to create the PR to show other contributors the proposed implementation, but I'm open to different approaches.

Change Type

Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance

Is this a breaking change?

Yes
No

How did you test this PR?

Added unit tests.

Also tested with an existing OTEL processing pipeline, replacing an existing http_server source (see below) with an opentelemetry source using the new http.headers config from this change.

Also ran a wrk load test to verify that performance is much higher with this solution than with an http_source that looks like this:

[sources.opentelemetry_logs_http_server]
type = "http_server"
address = "0.0.0.0:14318"
path = "/v1/logs"
decoding.codec = "protobuf"
decoding.protobuf.desc_file = "/path/to/opentelemetry-proto.desc"
decoding.protobuf.message_type = "opentelemetry.proto.collector.logs.v1.ExportLogsServiceRequest"
headers = [
    # To get access to the client IP address. 
    "x-forwarded-for",
]

See the linked issue (#21673) for more info on the performance impact of this new feature.

Does this PR include user facing changes?

Yes. Please add a changelog fragment based on our guidelines.
No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

Please read our Vector contributor resources.

References

Closes: #21673

jblazquez · 2024-11-01T01:31:35Z

src/sources/opentelemetry/http.rs

@@ -103,12 +114,69 @@ pub(crate) fn build_warp_filter(
    log_filters.or(trace_filters).unify().boxed()
 }

+fn enrich_events(


This function is similar to SimpleHttpSource::enrich_events, but it only injects headers into the event.

It looks like the code blocks that injects the headers are identical (except for the source names). I think it would be nice to convert this into a common function and move it to a new file src/sources/util/http_common.rs.

jblazquez · 2024-11-01T01:33:06Z

src/sources/opentelemetry/http.rs


 use crate::http::{KeepaliveConfig, MaxConnectionAgeLayer};
+use crate::sources::http_server::{HttpConfigParamKind, SimpleHttpConfig};


Not entirely sure if it's OK to use types from the source::http_server module here, but other sources (e.g. heroku_logs) also do it, so I assumed it is OK.

We can also move HttpConfigParamKind into the common file.

I decided against making this change for now, as it touches a number of unrelated files and it wasn't super clear which common file to move this type to.

src/sources/opentelemetry/tests.rs

pront

Hi @jblazquez, thank you for this contribution!

pront · 2024-11-01T20:08:10Z

src/sources/opentelemetry/http.rs

+                                        headers_config.get(header_name).map(HeaderValue::as_bytes);
+
+                                    log_namespace.insert_source_metadata(
+                                        SimpleHttpConfig::NAME,


Replace with OpentelemetryConfig (or this can also resolved by the refactoring I suggested above).

pront · 2024-11-01T20:11:07Z

src/sources/opentelemetry/http.rs

@@ -103,12 +114,69 @@ pub(crate) fn build_warp_filter(
    log_filters.or(trace_filters).unify().boxed()
 }

+fn enrich_events(


It looks like the code blocks that injects the headers are identical (except for the source names). I think it would be nice to convert this into a common function and move it to a new file src/sources/util/http_common.rs.

src/sources/opentelemetry/tests.rs

…nflicting fields

jblazquez · 2024-11-02T03:38:15Z

@pront, thank you for the review. Now that I'm more confident about these changes, I reworked the PR to address your feedback:

516d1da: Creates a new sources::util::http::headers module and adds an add_headers function there. I tried to follow the existing pattern for the sources::util::http::query::add_query_parameters function, including the Cargo features.
36895e3: Modifies the http_server source to use the new add_headers function.
43f474f: Modifies the opentelemetry source to use the new add_headers function, adds tests, and updates the documentation and changelog.
0bf233c: Corrects the description of the behavior of the headers option in http_server. Unlike query_parameters, the headers do not overwrite existing fields with the same name. Added a test to confirm the correct behavior.

In addition to the commits above, I also added the following commit that extends this PR to also bring support for the host_key configuration to the opentelemetry source, for more consistency with http_server. If you think this change should be done in a different PR please let me know, but I felt it was closely related to the headers feature.

ea5c56f: Adds support for the host_key config to opentelemetry. I followed the existing pattern from http_server for plumbing through the remote IP.

pront · 2024-11-04T15:24:26Z

src/sources/opentelemetry/http.rs

+        let remote_addr = conn.peer_addr();
+        let remote_addr_ref = enable_source_ip.then_some(remote_addr);


The following is preferable because it's Some only enable_source_ip is true:

Suggested change

let remote_addr = conn.peer_addr();

let remote_addr_ref = enable_source_ip.then_some(remote_addr);

let remote_addr = enable_source_ip.then(|| conn.peer_addr());

Thanks, if I come back to this work to add host_key to opentelemetry I will do this. I had copied what the http_server source did.

pront · 2024-11-04T15:29:03Z

changelog.d/21673_add_http_headers_to_opentelemetry_source.feature.md

@@ -0,0 +1,3 @@
+The `opentelemetry` source can now be configured to enrich log events with the source IP and any HTTP headers received in the OTLP/HTTP request.


Let's also mention that now a host_key can be specified.

Note, I am skeptical about the connection of the host_key with source_ip. I understand there's precedence in other sources but I can see an argument in favor of making it a different setting.

Commit ea5c56f mentioned this, but I reverted that commit to reduce the scope of this PR, since you said you were not sure that host_key should mean source_ip.

pront · 2024-11-04T15:33:31Z

src/sources/opentelemetry/http.rs

+                if let Some(remote_addr_inner) = remote_addr_ref.as_ref() {
+                    request.extensions_mut().insert(*remote_addr_inner);
+                }


Suggested change

if let Some(remote_addr_inner) = remote_addr_ref.as_ref() {

request.extensions_mut().insert(*remote_addr_inner);

}

if let Some(addr) = remote_addr {

request.extensions_mut().insert(addr);

}

pront · 2024-11-04T15:40:20Z

src/sources/opentelemetry/tests.rs

+            .unwrap()
+            .is_timestamp());
+        assert_eq!(
+            meta.get(path!("opentelemetry", "host")).unwrap(),


I would expect this to be remote_ip. I think this is also broken in http_server because we hardcode path!("host") when enriching events.

I also wondered this, but I assumed it was intentional that - when using log namespacing - the source IP was always under path!("host") and that the host_key configuration was only for the legacy namespace.

But you bring up some good questions about the behavior of this setting, so I reverted the host_key change from this PR.

pront · 2024-11-04T15:41:35Z

@pront, thank you for the review. Now that I'm more confident about these changes, I reworked the PR to address your feedback:

516d1da: Creates a new sources::util::http::headers module and adds an add_headers function there. I tried to follow the existing pattern for the sources::util::http::query::add_query_parameters function, including the Cargo features.

36895e3: Modifies the http_server source to use the new add_headers function.

43f474f: Modifies the opentelemetry source to use the new add_headers function, adds tests, and updates the documentation and changelog.

0bf233c: Corrects the description of the behavior of the headers option in http_server. Unlike query_parameters, the headers do not overwrite existing fields with the same name. Added a test to confirm the correct behavior.

In addition to the commits above, I also added the following commit that extends this PR to also bring support for the host_key configuration to the opentelemetry source, for more consistency with http_server. If you think this change should be done in a different PR please let me know, but I felt it was closely related to the headers feature.

ea5c56f: Adds support for the host_key config to opentelemetry. I followed the existing pattern from http_server for plumbing through the remote IP.

Thank you @jblazquez, much appreciated 👍

Adding the host_key introduced new scope so I did another review and some new things came up.

jblazquez · 2024-11-04T18:03:37Z

@pront Appreciate the review!

You brought up some good questions about how host_key should behave, so I did not want to add that new scope to this PR. I reverted the commit that added host_key so this PR is purely about the headers config.

Let me know if there are other changes needed. Thanks!

pront · 2024-11-05T16:06:57Z

@pront Appreciate the review!

You brought up some good questions about how host_key should behave, so I did not want to add that new scope to this PR. I reverted the commit that added host_key so this PR is purely about the headers config.

Let me know if there are other changes needed. Thanks!

Sounds great. Will review again today.

If you are still interested in adding a host_key, feel free to submit another after this one is merged. I think we have a good understanding of how it should work.

pront

Thank you @jblazquez!

feat(opentelemetry): add support for enriching logs with HTTP headers

7c2ba8c

jblazquez requested a review from a team as a code owner November 1, 2024 01:29

github-actions bot added the domain: sources Anything related to the Vector's sources label Nov 1, 2024

jblazquez commented Nov 1, 2024

View reviewed changes

src/sources/opentelemetry/tests.rs Show resolved Hide resolved

pront added the source: opentelemetry Anything `opentelemetry` source related label Nov 1, 2024

pront self-assigned this Nov 1, 2024

pront reviewed Nov 1, 2024

View reviewed changes

jblazquez added 4 commits November 1, 2024 17:17

Add sources::util::http::headers module

516d1da

http_server: Use sources::util::http::add_headers

36895e3

opentelemetry: Use sources::util::http::add_headers

43f474f

http_server: Correct description of header injection behavior with co…

0bf233c

…nflicting fields

jblazquez requested review from a team as code owners November 2, 2024 03:31

github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Nov 2, 2024

pront reviewed Nov 4, 2024

View reviewed changes

drichards-87 approved these changes Nov 4, 2024

View reviewed changes

jblazquez changed the title ~~feat(opentelemetry): add support for enriching logs with HTTP headers~~ feat(opentelemetry source): add support for enriching logs with HTTP headers Nov 4, 2024

jblazquez force-pushed the opentelemetry-http-headers branch from ea5c56f to 0bf233c Compare November 4, 2024 17:59

pront approved these changes Nov 5, 2024

View reviewed changes

pront enabled auto-merge November 5, 2024 16:12

pront added this pull request to the merge queue Nov 5, 2024

Merged via the queue into vectordotdev:master with commit 872d0b5 Nov 5, 2024
97 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opentelemetry source): add support for enriching logs with HTTP headers #21674

feat(opentelemetry source): add support for enriching logs with HTTP headers #21674

jblazquez commented Nov 1, 2024 •

edited

Loading

jblazquez Nov 1, 2024

pront Nov 1, 2024 •

edited

Loading

jblazquez Nov 1, 2024 •

edited

Loading

pront Nov 1, 2024

jblazquez Nov 2, 2024 •

edited

Loading

pront left a comment

pront Nov 1, 2024

pront Nov 1, 2024 •

edited

Loading

jblazquez commented Nov 2, 2024

pront Nov 4, 2024

jblazquez Nov 4, 2024

pront Nov 4, 2024

jblazquez Nov 4, 2024

pront Nov 4, 2024

pront Nov 4, 2024

jblazquez Nov 4, 2024

pront commented Nov 4, 2024

jblazquez commented Nov 4, 2024

pront commented Nov 5, 2024

pront left a comment


		use crate::http::{KeepaliveConfig, MaxConnectionAgeLayer};
		use crate::sources::http_server::{HttpConfigParamKind, SimpleHttpConfig};

		let remote_addr = conn.peer_addr();
		let remote_addr_ref = enable_source_ip.then_some(remote_addr);

	let remote_addr = conn.peer_addr();
	let remote_addr_ref = enable_source_ip.then_some(remote_addr);
	let remote_addr = enable_source_ip.then(\|\| conn.peer_addr());

		@@ -0,0 +1,3 @@
		The `opentelemetry` source can now be configured to enrich log events with the source IP and any HTTP headers received in the OTLP/HTTP request.

feat(opentelemetry source): add support for enriching logs with HTTP headers #21674

feat(opentelemetry source): add support for enriching logs with HTTP headers #21674

Conversation

jblazquez commented Nov 1, 2024 • edited Loading

Summary

Change Type

Is this a breaking change?

How did you test this PR?

Does this PR include user facing changes?

Checklist

References

Choose a reason for hiding this comment

pront Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

jblazquez Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jblazquez Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

pront left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pront Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

jblazquez commented Nov 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pront commented Nov 4, 2024

jblazquez commented Nov 4, 2024

pront commented Nov 5, 2024

pront left a comment

Choose a reason for hiding this comment

jblazquez commented Nov 1, 2024 •

edited

Loading

pront Nov 1, 2024 •

edited

Loading

jblazquez Nov 1, 2024 •

edited

Loading

jblazquez Nov 2, 2024 •

edited

Loading

pront Nov 1, 2024 •

edited

Loading