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

Auto-detect service name from web.xml display-name #886

Merged
merged 24 commits into from Sep 2, 2022
Merged

Auto-detect service name from web.xml display-name #886

merged 24 commits into from Sep 2, 2022

Conversation

laurit
Copy link
Collaborator

@laurit laurit commented Aug 23, 2022

No description provided.

@laurit laurit marked this pull request as ready for review August 24, 2022 09:32
@laurit laurit requested review from a team as code owners August 24, 2022 09:32
laurit
laurit commented Aug 26, 2022
View reviewed changes
custom/src/main/java/com/splunk/opentelemetry/SplunkConfiguration.java Outdated
if (ServiceNameDetector.serviceNameNotConfigured(config)) {
String serviceName = ServiceNameDetector.detectServiceName();
if (serviceName != null) {
log.info("Auto-detected service name '{}'.", serviceName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we also log some kind of description (path to the web.xml?) based on what we auto-detected?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not too much trouble, then why not -- it'd make our lives easier. I'd log it at debug though, some customers are kinda sensitive to the amount of logs printed by the agent.

laurit
laurit commented Aug 26, 2022
View reviewed changes
custom/src/main/java/com/splunk/opentelemetry/servicename/TomeeServiceNameDetector.java

@Override
Path getDeploymentDir() throws URISyntaxException {
Path rootDir = getRootDir();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

our test uses webapps directory so this isn't tested

mateuszrzeszutek
mateuszrzeszutek reviewed Aug 26, 2022
View reviewed changes
Copy link
Contributor

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

This is awesome 👍
Even if it's only a fraction of all possible ways to deploy servlet apps, I think it's more than enough to get us started with this -- all other cases would be covered on demand (or not at all, if they're too insane)

custom/src/main/java/com/splunk/opentelemetry/SplunkConfiguration.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/ServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/AppServerServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/AppServerServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/AppServerServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/TomeeServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/TomeeServiceNameDetector.java Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/LibertyServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/WebSphereServiceNameDetector.java Outdated Show resolved Hide resolved
custom/src/main/java/com/splunk/opentelemetry/servicename/WebSphereServiceNameDetector.java Outdated Show resolved Hide resolved
breedx-splk
breedx-splk reviewed Aug 29, 2022
View reviewed changes
custom/src/test/java/com/splunk/opentelemetry/servicename/AppServerServiceNameDetectorTest.java
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

public class AppServerServiceNameDetectorTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is really a unit test because it does so much with the environment and tests across detectors. Feels like an integration test to me. Is there a better home for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not test across detectors. It only test logic in the base detector class.

breedx-splk
breedx-splk approved these changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Agree with others that this is fantastic! I would have loved to see it broken up into more bite sized PRs for reviewing, but great anyway. 👍🏻

mateuszrzeszutek
mateuszrzeszutek approved these changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

custom/src/main/java/com/splunk/opentelemetry/servicename/ServiceNameResourceProvider.java Outdated Show resolved Hide resolved
@mateuszrzeszutek mateuszrzeszutek merged commit 653586e into signalfx:main Sep 2, 2022
@laurit laurit deleted the autodetect-service-name branch September 2, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breedx-splk breedx-splk breedx-splk approved these changes

@theletterf theletterf theletterf approved these changes

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@laurit @theletterf @mateuszrzeszutek @breedx-splk