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

OTLP tonic metadata from env variable #1377

Merged
merged 8 commits into from
Nov 19, 2023

Conversation

harscoet
Copy link
Contributor

@harscoet harscoet commented Nov 16, 2023

Fixes #1336

Changes

As per the specs, the custom headers for OTLP exporter can be specified through env variables - OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TRACES_HEADERS, OTEL_EXPORTER_OTLP_METRICS_HEADERS.
This PR completes the work already done in PR #1290 adding support for tonic metadata
To reproduce the same behavior as http exporter, the env-variable takes precedence (as discussed in open-telemetry/opentelemetry-rust-contrib#10)

I moved common code for http and tonic exporters in exporter/mod.rs (function to parse header from string and test helper to run tests with isolated env variables)
I wanted to minimize the changes but maybe it should be a good idea to use a crate like https://crates.io/crates/temp-env for environment related testing

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@harscoet harscoet requested a review from a team November 16, 2023 12:36
Copy link

linux-foundation-easycla bot commented Nov 16, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (c06c04b) 55.4% compared to head (76c0573) 55.6%.
Report is 2 commits behind head on main.

Files Patch % Lines
opentelemetry-sdk/src/metrics/periodic_reader.rs 0.0% 6 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/mod.rs 97.7% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1377     +/-   ##
=======================================
+ Coverage   55.4%   55.6%   +0.2%     
=======================================
  Files        147     145      -2     
  Lines      18083   18022     -61     
=======================================
+ Hits       10021   10037     +16     
+ Misses      8062    7985     -77     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@shaun-cox shaun-cox left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

}
for (key, value) in parse_header_string(input) {
if let (Ok(key), Ok(value)) = (HeaderName::from_str(key), HeaderValue::from_str(value)) {
headers.insert(key, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I love the cleanup. I wonder if one more little step here could eliminate the for loop by leveraging the std::iter::Extend trait that HashMap implements? Something like:

headers.extend(
    parse_header_string(input)
        .filter_map(|(k,v)| { 
           let key = HeaderName::from_str(k)?;
           let value = HeaderName::from_str(v)?;
           Some((key, value))
        )
    );

(I may have the .filter_map lambda totally wrong, but hopefully the gist comes across.

Copy link
Contributor Author

@harscoet harscoet Nov 18, 2023

Choose a reason for hiding this comment

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

Good suggestion, I updated it

fn add_header_from_string(input: &str, headers: &mut HashMap<HeaderName, HeaderValue>) {
    headers.extend(parse_header_string(input).filter_map(|(key, value)| {
        Some((
            HeaderName::from_str(key).ok()?,
            HeaderValue::from_str(value).ok()?,
        ))
    }));
}

@harscoet
Copy link
Contributor Author

harscoet commented Nov 18, 2023

Both http and tonic modules are using HeaderName/HeaderValue so I just pushed some updates to simplify the code of parse_header_string to return impl Iterator<Item = (HeaderName, HeaderValue)> + '_ (instead of impl Iterator<Item = (&str, &str)>)

@hdost
Copy link
Contributor

hdost commented Nov 18, 2023

🤕 latest tests fail.

@harscoet
Copy link
Contributor Author

🤕 latest tests fail.

There seem to be random failures during opentelemetry-jaeger tests, due to environment variable updates from parallel tests.
I created a PR #1384

@harscoet
Copy link
Contributor Author

harscoet commented Nov 19, 2023

Both http and tonic modules are using HeaderName/HeaderValue so I just pushed some updates to simplify the code of parse_header_string to return impl Iterator<Item = (HeaderName, HeaderValue)> + '_ (instead of impl Iterator<Item = (&str, &str)>)

I finally reverted to use impl Iterator<Item = (&str, &str)> after realizing that grpcio is using HashMap<String, String> for headers, it's better to stay generic for all exporters

For grpcio we will be able to use something like that

fn add_headers_from_string(input: &str, headers: &mut HashMap<String, String>) {
    headers.extend(
        parse_header_string(&input).map(|(key, value)| (key.to_string(), value.to_string())),
    );
}

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Nicely done. Thanks.

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Thank you again!

@hdost hdost merged commit 9e2e3db into open-telemetry:main Nov 19, 2023
15 checks passed
@harscoet harscoet deleted the otlp-tonic-metadata-from-env branch November 20, 2023 14:26
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.

OTLP gRPC exporter doesn't support OTEL_EXPORTER_OTLP_HEADERS but OTLP/HTTP supports it.
5 participants