-
Notifications
You must be signed in to change notification settings - Fork 867
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
Add resource provider for CloudFoundry #12862
base: main
Are you sure you want to change the base?
Add resource provider for CloudFoundry #12862
Conversation
Adds a module to provide a resource provider for applications deployed to CloudFoundry. It parses the VCAP_APPLICATION environment variable to fill in the attributes specified in the semantic conventions[1]. The implementation follows the OsResource in the resources module. [1] https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/cloudfoundry.md Signed-off-by: Karsten Schnitter <[email protected]>
This change addresses #12861. Since this is my first contribution, please let me know, if I violated any guidelines. I was unsure, whether this is a library or javaagent instrumentation. Note, this feature can be enhanced by adding a service name detector, that provides the |
Aws resource and gcp resource providers are in opentelemetry-java-contrib repository.
Javaagent instrumentations are embedded inside the agent and only work with the agent, library instrumentations can be added to the application. Resource providers are like library instrumentations, they can be used without the agent. |
@laurit thanks for the clarification. Are you suggesting, I take this PR to opentelemetry-java-contrib? |
Do I understand, that I would add the code to a submodule of opentelemetry-java-contrib and reference this at opentelemetry-java-instrumentation/dependencyManagement/build.gradle.kts Lines 102 to 105 in 4f074ec
|
Besides adding it to the dependency management you would also need to add it to
Line 24 in 96eccaf
Since this resource provider is considerably simpler than gcp and aws another option that could be considered is having it in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/resources/library @trask what is your preference? |
I think I prefer for it to have its own module (instead of adding to probably contrib is the most consistent with the existing cloud resource providers, so I'm 👍 that route |
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.
Oops, forgot to "Submit review" a few days ago, sorry...
CloudFoundryAttributesBuilder attributes = new CloudFoundryAttributesBuilder(vcapAppRaw); | ||
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.
nit: I would name the variable builder
instead of 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 have to strong feelings about that. My thoughts were to make it clear, that only the attributes are being built, not the entire resource.
.../io/opentelemetry/javaagent/instrumentation/cloudfoundry/resources/CloudFoundryResource.java
Outdated
Show resolved
Hide resolved
@CanIgnoreReturnValue | ||
private CloudFoundryAttributesBuilder putString(AttributeKey<String> key, String name) { | ||
Object value = parsedData.get(name); | ||
if (value instanceof String) { | ||
builder.put(key, (String) value); | ||
} | ||
return this; | ||
} | ||
|
||
@CanIgnoreReturnValue | ||
private CloudFoundryAttributesBuilder putNumberAsString(AttributeKey<String> key, String name) { | ||
Object value = parsedData.get(name); | ||
if (value instanceof Number) { | ||
builder.put(key, value.toString()); | ||
} | ||
return this; | ||
} |
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 feels like a lot of extra care to avoid just using toString()
for every value...I wonder why.
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.
value
could be null. I can refactor the code to a null check plus toString()
if you prefer that. Let me know.
been there, and I think it's really weird how GitHub shows the comments with the date you made them instead of the date you posted them |
…c/main/java/io/opentelemetry/javaagent/instrumentation/cloudfoundry/resources/CloudFoundryResource.java Co-authored-by: jason plumb <[email protected]>
The same discussion is going on in #12861, the enhancement proposal for this PR. I have no problem taking this code to the contrib repository. But I still want to point out, that CloudFoundry is open source and governed by the CloudFoundry Foundation a member of the Linux Foundation. This is a difference to cloud providers like AWS or GCP. If you still think, this resource provider should go to the contrib repository, let me know. I will move it to the repository if necessary. |
we may revisit this in the future open-telemetry/opentelemetry-java-contrib#1603 (comment)
but for now I believe the best place is in the contrib repo where we have a different component ownership model and so you can continue to own the module |
Adds a module to provide a resource provider for applications deployed to CloudFoundry. It parses the VCAP_APPLICATION environment variable to fill in the attributes specified in the semantic conventions[1].
The implementation follows the OsResource in the resources module.
[1] https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/cloudfoundry.md