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

Add Spring Boot service name guesser / ResourceProvider #427

Closed
wants to merge 22 commits into from

Conversation

breedx-splk
Copy link
Contributor

This may not be the final home for ResourceProvider implementations, but it's a start. I know that the core project is likely going to migrate at least some of the existing ResourceProviders out of core....maybe some go to the instrumentation repo and some go to contrib? I'm open to reorganization suggestions.

This adds a new module that contains a single ResourceProvider called SpringBootServiceNameGuesser that does what it says. There are some additional, more complicated strategies that have not yet been included (such as the inline JSON doc as environment variable).

This new module will publish a new contrib artifact that distros can include to do spring app name detection.

This class could have been built as a composition of each of the strategies with a Resource.merge() step at the end, but that felt a little overengineered.

@breedx-splk breedx-splk requested a review from a team August 23, 2022 16:56
@mateuszrzeszutek
Copy link
Member

I'm in favor of including this in the base javaagent distro -- it looks like it'll be very useful for spring boot users, and the accuracy of guessing the service name is rather high I think.

}

InputStream openFile(String filename) throws Exception {
return Files.newInputStream(Paths.get(filename));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would check that the file exists before attempting to open it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why, just to avoid the FileNotFound exception? I'm pretty "meh" on this.

I mean, if we're going to do that we might as well check to ensure that the file exists AND that it's a regular file AND that I have permission to read it also, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be super annoying if you need to debug some exception during agent startup and use an exception breakpoint and you get a million random exceptions before the one where you want to stop. Imo it is best not to cause exceptions if you can easily avoid it. I'd add regular file check, but I wouldn't bother with checking permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it more annoying that code that's just littered with checked exception handlers and guarded code all over the place? 😁

In this case, if I add check, what should I do in the event that the file doesn't exist?

Apparently I shouldn't throw a different exception, so I guess I slap @Nullable on the method and return a null InputStream? That will cause an NPE in the caller unless guarded, and the callers are currently using try-with-resources. So I'll have to unwrap those into separate assignments. Yuck. Alternatively, I could return a ByteArrayInputStream with nothing in it and hope that the parsers can grok an empty stream without exploding -- but returning an empty stream also suggests to any caller that the file was actually read but contains nothing. That's not great either. I dunno. Situations like this always feel like a language shortcoming to me.

Coincidentally, I was reading the nodejs docs on fs.exists() yesterday and they explicitly discourage checking before attempting because it's not multi-thread/process safe. That doesn't really apply here, but I thought it was interesting nonetheless.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know try with resource works nicely with null, it won't NPE.

@trask
Copy link
Member

trask commented Aug 26, 2022

I'm in favor of including this in the base javaagent distro -- it looks like it'll be very useful for spring boot users, and the accuracy of guessing the service name is rather high I think.

+1

@breedx-splk breedx-splk marked this pull request as draft August 26, 2022 16:49
@breedx-splk
Copy link
Contributor Author

Closing and will reopen in the main instrumentation repo.

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