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

Webapp service name detector #562

Merged
merged 17 commits into from
Nov 9, 2022

Conversation

breedx-splk
Copy link
Contributor

@breedx-splk breedx-splk commented Nov 1, 2022

This is the start of a few contributions of ResourceDetector implementations from the Splunk distro. The first one coming up here is the AppServerServiceNameProvider. This will eventually detect when the service.name resource attribute is missing (and also otel.service.name not set in config) and will apply a series of heuristics for common application servers.

Because the current implementation is not exactly small, it's being contributed piecemeal to make reviews easier. The implementation here doesn't actually apply any of the heuristics yet....to see what they'll look like when done, you can look in the splunk distribution impl.

@breedx-splk breedx-splk marked this pull request as ready for review November 1, 2022 22:56
@breedx-splk breedx-splk requested a review from a team November 1, 2022 22:56
breedx-splk and others added 3 commits November 2, 2022 09:09
…iders/AppServerServiceNameProvider.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
…iders/AppServerServiceNameProvider.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
@github-actions github-actions bot requested a review from laurit November 2, 2022 16:30
resource-providers/README.md Outdated Show resolved Hide resolved
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.resourceproviders;
Copy link
Member

Choose a reason for hiding this comment

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

can all of these classes go under an internal package, or do users need access to them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the AppServerServiceNameProvider will be the only real user-facing class (although the delegates could be too I suppose, if someone wanted to pull in only a subset of the supported app servers). For now, I think I should make the two interfaces package-private and that should help, right?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

For now, I think I should make the two interfaces package-private and that should help, right?

looks good, we can revisit visibility issues later too, thx

@trask trask merged commit 16266e2 into open-telemetry:main Nov 9, 2022
trask pushed a commit that referenced this pull request Nov 15, 2022
This is a continuation of #562 that introduces the first of several web
app servers for which we will detect the service name.

Because this is the first (GlassFish), it includes some of the
helper/plumbing utilities. Subsequent additions will mostly be the
`AppServer` implementations (and adding them to the delegate list).
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.

4 participants