-
Notifications
You must be signed in to change notification settings - Fork 33
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
Allow setting otlp protocol for profiler with otel.exporter.otlp.protocol #2090
Conversation
return config.getString( | ||
CONFIG_KEY_PROFILER_OTLP_PROTOCOL, | ||
config.getString(CONFIG_KEY_OTLP_PROTOCOL, "http/protobuf")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -82,7 +83,6 @@ Map<String, String> defaultProperties() { | |||
config.put(CONFIG_KEY_MEMORY_ENABLED, String.valueOf(DEFAULT_MEMORY_ENABLED)); | |||
config.put(CONFIG_KEY_MEMORY_EVENT_RATE, DEFAULT_MEMORY_EVENT_RATE); | |||
config.put(CONFIG_KEY_CALL_STACK_INTERVAL, DEFAULT_CALL_STACK_INTERVAL.toMillis() + "ms"); | |||
config.put(CONFIG_KEY_OTLP_PROTOCOL, "http/protobuf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it a little more challenging for a casual reader to know the default, but I think it's fine.
void getOtlpProtocolDefault() { | ||
String result = | ||
Configuration.getOtlpProtocol(DefaultConfigProperties.create(Collections.emptyMap())); | ||
assertEquals(result, "http/protobuf"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order of arguments should be reversed. Currently assertion error message may be misleading if the assertion fails.
The same applies to all assetEquals calls in this file.
Maybe using assertThat(result).isEqualTo("http/protobuf");
would be little bit more verbose, but on the other hand not confusing.
Use
otel.exporter.otlp.protocol
whensplunk.profiler.otlp.protocol
is not set.