Skip to content

Add instrumentation to detect the route at the beginning of the spring request#1360

Merged
tylerbenson merged 2 commits into
masterfrom
tyler/spring-dispatcher
Apr 14, 2020
Merged

Add instrumentation to detect the route at the beginning of the spring request#1360
tylerbenson merged 2 commits into
masterfrom
tyler/spring-dispatcher

Conversation

@tylerbenson
Copy link
Copy Markdown
Contributor

Instead of waiting till the handler is called, otherwise if a response is returned by a filter then the proper name wouldn't be set and would fall back to the URL.

I tried to add comments for the parts that are interdependent. Let me know if it's insufficient.

@tylerbenson tylerbenson requested a review from a team as a code owner April 9, 2020 18:55
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this doesn't have to implement Filter since it's not being used as one, but since we don't have access to the functional interfaces there doesn't really seem to be a better type in my mind

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was my thought also... If there's a better generic interface that takes a single argument I'd be happy to use that instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want log here? Add a health metric?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a log statement, even though the exception is never thrown. If we had a better shared interface we could avoid the try/catch (filter doesn't actually need to implement the Filter interface).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I realize this didn't change in this PR, but I'll ask anyway.

I'd expect scope.close to be inside of a finally block, but usually, it is not.
I'd actually expect XDecorator.beforeFinish to be inside a finally block as well.

Maybe, the first two lines don't typically raise exceptions; however, we might not know because of the suppress=Throwable.

Basically, I'm concerned that this code is not obviously correct. From looking around, this seems to be a general problem with our resource handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's true that exceptions thrown from our decorators could cause problems in many places... That should probably be addressed as a separate issue.

…g request

Instead of waiting till the handler is called, otherwise if a response is returned by a filter then the proper name wouldn't be set and would fall back to the URL.
@tylerbenson tylerbenson force-pushed the tyler/spring-dispatcher branch from 2db1e17 to ada2fdf Compare April 14, 2020 18:31
@tylerbenson tylerbenson force-pushed the tyler/spring-dispatcher branch from ada2fdf to ce006e1 Compare April 14, 2020 19:27
@tylerbenson tylerbenson merged commit d313d71 into master Apr 14, 2020
@tylerbenson tylerbenson deleted the tyler/spring-dispatcher branch April 14, 2020 20:30
@tylerbenson tylerbenson added this to the 0.49.0 milestone Apr 15, 2020
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.

3 participants