-
Notifications
You must be signed in to change notification settings - Fork 41k
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
Support OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES environment variables #44394
Conversation
f84e68e
to
5fdf53a
Compare
Converted it to a draft until #44400 is resolved. |
4c0e932
to
e27117b
Compare
The PR has been updated, taking into account |
eb0a823
to
573e67b
Compare
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
* @param value value to decode | ||
* @return the decoded string | ||
*/ | ||
public static String decode(String value) { |
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.
I reviewed various SDKs, and each has a different approach to parsing OTEL_RESOURCE_ATTRIBUTES
environment variable:
- opentelemetry-dotnet: does not decode values at all.
- opentelemetry-java: uses
java.net.URLDecoder
for decoding, where '+' is treated as a space (' '). - opentelemetry-python: decodes values the same way as the Java SDK.
- opentelemetry-go: uses
url.PathUnescape(strings.TrimSpace(v))
for decoding, treating '+' as a literal '+'.
I've not found information, that '+' should be decoded as a space (' ').
I was trying to implement parsing OTEL_RESOURCE_ATTRIBUTES
environment variable based on the following:
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.
Yeah, the decoder looks dubious there.
- as I know "implementation was borrowed from {@code org.apache.commons.codec.net.PercentCodec}". Reusing is always better then "borrowing".
- stick to existing approach if it's not compromised yet. Replacing '+' to spaces seems to be simler then decoding. standardization makes life better. Might be
java.net.URLDecoder
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.
Russian lang is supported
* @param value value to decode | ||
* @return the decoded string | ||
*/ | ||
public static String decode(String value) { |
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.
Yeah, the decoder looks dubious there.
- as I know "implementation was borrowed from {@code org.apache.commons.codec.net.PercentCodec}". Reusing is always better then "borrowing".
- stick to existing approach if it's not compromised yet. Replacing '+' to spaces seems to be simler then decoding. standardization makes life better. Might be
java.net.URLDecoder
If you have defined your own javadoc:io.opentelemetry.sdk.resources.Resource[] bean, this will no longer be the case. | ||
|
||
NOTE: Spring Boot does not provide auto-configuration for OpenTelemetry metrics or logging. | ||
OpenTelemetry tracing is only auto-configured when used together with xref:actuator/tracing.adoc[Micrometer Tracing]. | ||
|
||
NOTE: The `OTEL_RESOURCE_ATTRIBUTES` environment variable consist of a list of key-value pairs. For example: `key1=value1,key2=value2,key3=spring%20boot`. All attribute values are treated as strings, and any characters outside the baggage-octet range must be **percent-encoded**. |
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.
Why encoding? Escaping seems more readable
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.
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.
Reference to specs is respectful.
But there're lots spesc prescribing opposite.
I think the following considerations should define what standard to follow:
- What standard is already chosen and specified for our context(openTelemetry or Java)
- What standard is more prefferable for guys reading(not writing) the attributes.
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.
I'm not sure how https://docs.oracle.com/javase/specs/jls/se20/html/jls-3.html#jls-3.10.6 relates to this PR.
However, to clarify, I used the term "percent-encoded" instead of "escaped/escaping" or any other variation because, according to RFC 3986, it is named Percent-Encoding.
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.
I wont insist on backslash-escaping. Let me just explain my point.
The guys who'll handle the variable are DevOps. DevOps would like to:
- maintain the property unfomly for all applications(regardless lang)
- use familiarsytax(less important)
As for the first, only a way to make the property format consistent throughout implementations is writing it in the official docs. Unfortunately the case is not documented there yet. So only a way is to predict the aspect relying on secondary considerations.
As for the second, it's valid to expect that DevOps teams would prefer to use standard CLI syntax conforming POSIX spec.
And my final argument for backslash-encoding.
Properties from environment variables could be overriden with configuration(json, yaml) or a string variable.
And surprizingly JSON, YAML and majority of langs are following backslash-escaping pattern.
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.
Unfortunately the case is not documented there yet.
I mean no disrespect and I could be mistaken but from my perspective, the responses seem auto-generated. With that said, I will stop responding to further comments and wait for feedback from the Spring Boot team. Thank you anyway.
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.
Now see. Thx, you're right
properties.getResourceAttributes().forEach(builder::put); | ||
Map<String, String> attributes = new OpenTelemetryResourceAttributes(properties.getResourceAttributes()) | ||
.asMap(); | ||
attributes.computeIfAbsent("service.name", (key) -> getApplicationName(environment)); |
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.
"service.name" is declared in OpenTelemetryResourceAttributes
too.
Better to reuse a public constant from OpenTelemetryResourceAttributes
.
Even better to reuse a constant from opentelemetry SDK.
For "service.group" the same note
return builder.build(); | ||
} | ||
|
||
private String getApplicationName(Environment environment) { | ||
return environment.getProperty("spring.application.name", DEFAULT_APPLICATION_NAME); |
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.
Consider using a @value(${}) variable.
For "spring.application.group" too
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.
The use of @Value(${..})
is uncommon in the Spring Boot codebase. Since we already have access to the Environment
here, it is better to use it.
* | ||
* @author Dmytro Nosan | ||
*/ | ||
class OpenTelemetryResourceAttributesTests { |
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.
If you're going to decode parameters from "%a3%3e"-like expression still need a test case
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.
I have also included tests for this use case.
result.computeIfAbsent("service.name", (key) -> getApplicationName()); | ||
result.computeIfAbsent("service.group", (key) -> getApplicationGroup()); | ||
return Collections.unmodifiableMap(result); | ||
Map<String, String> attributes = new OpenTelemetryResourceAttributes( |
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.
Exact copy of config from OpenTelemetryAutoConfiguration. Consider reuse
@@ -95,17 +94,6 @@ public void setAggregationTemporality(AggregationTemporality aggregationTemporal | |||
this.aggregationTemporality = aggregationTemporality; | |||
} | |||
|
|||
@Deprecated(since = "3.2.0", forRemoval = true) |
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.
Do the "resource-attributes" have something to do with the issue? Why remove here?
Why not remove the property together?
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 property has been deprecated since 3.2.0
and is scheduled for removal in 3.5.0
. As I am making some changes in OtlpMetricsPropertiesConfigAdapter
, I believe it is appropriate to remove this property entirely now.
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.
Thanks for spotting this old deprecated code, @nosan. Given its deprecation in 3.2, it should have been removed in 3.4 but we must have missed it. I'd like it to be handled separately and I've opened #44468. I'll take care of it shortly. I hope rebasing on top of that change won't cause too much trouble.
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.
Given its deprecation in 3.2, it should have been removed in 3.4
Oops, I've been thinking in 3.5.0 🤦
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See spring-projectsgh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
This commit introduces the OpenTelementryAttributes class that fetches OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME environment variables and merges it with user-defined resource attributes. Besides that, this commit includes spec-compliant proper handling of OTEL_RESOURCE_ATTRIBUTES in OtlpMetricsPropertiesConfigAdapter and OpenTelemetryAutoConfiguration. See gh-44394 Signed-off-by: Dmytro Nosan <[email protected]>
Thanks a lot @nosan ! |
This update introduces support for OTEL-specific environment variables. The behavior now aligns with
OtlpMetricsPropertiesConfigAdapter
, which automatically handles this throughOtlpConfig.resourceAttributes
.The attribute order for
OpenTelemetryResourceAttributes
is as follows:management.opentelemetry.resource-attributes
OTEL_RESOURCE_ATTRIBUTES
andOTEL_SERVICE_NAME
environment variablesSee gh-43712
For consistency with
OtlpMetricsPropertiesConfigAdapter
, I do not merge attributes frommanagement.opentelemetry.resource-attributes
with attributes from environment variables.https://github.com/nosan/spring-boot/blob/fdcc8d9d1f632f00dd71093d604c374b6d0a38d3/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/export/otlp/OtlpMetricsPropertiesConfigAdapter.java#L79-L87