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.

Sign up for GitHub

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

Jump to bottom

enhance OTLP Exporter configuration #1373

Merged
merged 5 commits into from
Jun 28, 2020
Merged

enhance OTLP Exporter configuration #1373

merged 5 commits into from
Jun 28, 2020

Conversation

malafeev
Copy link
Contributor

OTLP Exporter in opentelemetry-java-instrumentation is not configurable to:

  • useTransportSecurity instead of usePlaintext
  • insert values into Metadata headers (in ClientInterceptor)

From PR open-telemetry/opentelemetry-java-instrumentation#567 (comment)
Decided to try add configuration directly to OTLP Exporter.

enhance OTLP Exporter configuration
f356998 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
@malafeev malafeev mentioned this pull request Jun 25, 2020
Closed
@codecov
Copy link

codecov bot commented Jun 25, 2020 •
edited
Loading

Codecov Report

Merging #1373 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1373   +/-   ##
=========================================
  Coverage     92.00%   92.00%           
  Complexity      888      888           
=========================================
  Files           114      114           
  Lines          3178     3178           
  Branches        262      262           
=========================================
  Hits           2924     2924           
  Misses          173      173           
  Partials         81       81

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24b4be9...59e7c9e. Read the comment docs.

jkwatson
jkwatson reviewed Jun 25, 2020
View reviewed changes
exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java
* </ul>
*
* <p>For environment variables, {@link OtlpGrpcSpanExporter} will look for the following names:
*
* <ul>
* <li>{@code OTEL_OTLP_SPAN_TIMEOUT}: to set the max waiting time allowed to send each span
* batch.
* <li>{@code OTEL_OTLP_ENDPOINT}: to set the endpoint to connect to.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a proposal in the specs to standardize environment variables for this sort of thing. Can you please add a comment to https://github.com/open-telemetry/opentelemetry-specification/pull/666/files with the needs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-specification#666 (comment)

@jkwatson
Copy link
Contributor

This looks good. We'll almost certainly need to update all of these env vars when the env var proposal gets merged.

@carlosalberto
Copy link
Contributor

Overall good, I definitely prefer to have it here than in the agent code. Two small things:

  1. Let's check for non-null values for setMetadata() (and maybe setEndpoint()? or specifying null would clear it up?)
  2. We need a clear, documented behavior for how to handle values when the user sets both the channel and endpoint. Does one override the other one? Does one of them has higher priority?

My preference would be to simplify things and remove the option to specify a ManagedChannel, but probably that's something an advanced user/scenario would need ;)

anuraaga
anuraaga reviewed Jun 26, 2020
View reviewed changes
exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java Outdated
private ManagedChannel channel;
private long deadlineMs = 1_000; // 1 second
private String endpoint;
private boolean useTls;
private Metadata metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java Outdated
private ManagedChannel channel;
private long deadlineMs = 1_000; // 1 second
private String endpoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java Outdated
if (metadataValue != null) {
for (String keyValueString : Splitter.on(';').split(metadataValue)) {
final List<String> keyValue = Splitter.on('=').splitToList(keyValueString);
;
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra semicolon

Copy link
Contributor Author

@malafeev malafeev Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it's still there...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed one more time

exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java
String metadataValue = getStringProperty(KEY_METADATA, configMap);
if (metadataValue != null) {
for (String keyValueString : Splitter.on(';').split(metadataValue)) {
final List<String> keyValue = Splitter.on('=').splitToList(keyValueString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Believe you can use Splitter.withKeyValueSeparator

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to create a static final with the splitter

`private static final Splitter.MapSplitter METADATA_PROPERTY_SPLITTER = Splitter.on(';').trimResults().omitEmptyStrings().withKeyValueSplitter('=');

Copy link
Contributor Author

@malafeev malafeev Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

withKeyValueSeparator is annotated by @Beta, should we use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof - good catch, I always forget that. Yeah let's not use it.

Sergei Malafeev added 2 commits June 26, 2020 14:57
add @nullable to nullable fields
4d8f9b3 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
add @nonnull to arguments of addHeader
6b6a7da 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
@malafeev
Copy link
Contributor Author

  • @carlosalberto I add @nullable and @nonnull annotations.
  • it would be great to get rid of explicit channel but who knows if it's needed by some case.
  • I added in the javadoc of setChannel method when channel is ignored and build using endpoint. I'm not sure where we can add more documentation because it's part of Builder only.

@carlosalberto
Copy link
Contributor

Let's try this out - it seems @anuraaga's feedback has been all addressed, so we are ready to merge this? ;)

@malafeev
Copy link
Contributor Author

I think so.

@trask
Copy link
Member

trask commented Jun 26, 2020

I'd suggest only annotating with @Nullable as a general rule (and treating non-null as the default)

Sergei Malafeev added 2 commits June 27, 2020 14:49
remove semicolon
42421f9 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
remove @nonnull
59e7c9e 
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
@malafeev
Copy link
Contributor Author

@trask I removed @Nonnull

@jkwatson jkwatson merged commit 3c144c7 into open-telemetry:master Jun 28, 2020
This was referenced Jun 29, 2020
Closed
Closed
@malafeev malafeev deleted the otlp-exporter-configuration branch June 30, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson approved these changes

@anuraaga anuraaga anuraaga left review comments

@thisthat thisthat thisthat approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@arminru arminru Awaiting requested review from arminru

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@malafeev @jkwatson @carlosalberto @trask @anuraaga @thisthat