Skip to content

Use path when high-level framework method is unknown - #1906

Merged
SylvainJuge merged 15 commits into
elastic:masterfrom
SylvainJuge:http-tx-priority-and-fallback
Jul 28, 2021
Merged

Use path when high-level framework method is unknown#1906
SylvainJuge merged 15 commits into
elastic:masterfrom
SylvainJuge:http-tx-priority-and-fallback

Conversation

@SylvainJuge

@SylvainJuge SylvainJuge commented Jul 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

HTTP Transaction naming can have multiple variants:

  • when using use_path_as_transaction_name we should use the request path and apply url_groups to limit cardinality
  • when using a high-level framework, the ClassName#methodName is preferred when use_path_as_transaction_name is false

Currently, the behavior when method name is not known, which happens with some Spring controllers, is to simply use the ClassName, which makes many distinct transactions be gathered under a common name.
Thus, in the case where the method name is null, using the path is often a better fallback.

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation

@ghost

ghost commented Jul 8, 2021 •
edited by ghost
Loading

Copy link
Copy Markdown

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-28T09:44:57.229+0000

  • Duration: 54 min 45 sec

  • Commit: f5bb2da

Test stats 🧪

Test Results
Failed 0
Passed 2374
Skipped 19
Total 2393

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2374
Skipped 19
Total 2393

@tobiasstadler

tobiasstadler commented Jul 9, 2021

Copy link
Copy Markdown
Contributor

@SylvainJuge I think TransactionNameUtils#setNameFromHttpRequestPath can also be used in co.elastic.apm.agent.httpserver.HttpHandlerAdvice

@SylvainJuge

SylvainJuge commented Jul 9, 2021

Copy link
Copy Markdown
Member Author

I've added ResourceHttpRequestHandler as a known exception, and thus name is not changed, we could also apply other naming schemes, we'll likely revisit this later if there is a need, a few ideas:

  • GET static resource for all static resources, which provide a bit more granularity with HTTP verb, could be reused for other frameworks too like unknown route.
  • GET *.js or GET *.css : apply extension-based grouping automatically,

@SylvainJuge
SylvainJuge marked this pull request as ready for review July 9, 2021 11:12
@SylvainJuge
SylvainJuge requested a review from eyalkoren July 9, 2021 11:14
eyalkoren
eyalkoren reviewed Jul 19, 2021
View reviewed changes

@eyalkoren eyalkoren left a comment

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.

Great refactoring!
I proposed an alternative approach for the actual fix (transaction naming in Spring MVC) that separates the transaction naming logic of the low-level framework from the high level framework.

Comment thread ...in/src/main/java/co/elastic/apm/agent/springwebmvc/SpringTransactionNameInstrumentation.java
@eyalkoren

eyalkoren commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

I applied the new APIs to Javalin and Vert.x and added a fallback option to use use_path_as_transaction_name for WebFlux transactions.

Ideally, the TransactionNameUtils API can be a bit nicer even:

  • not requiring the url_groups config, but currently we need it this way for the tests
  • the logic of setting unknown route unless the use_path_as_transaction_name config is set is quite common, so we kind or repeat it
  • maybe we can also share some of the logic of which priority to use in each case, but not sure

However, I don't know if it will eventually be better or if it worth the effort, only some points for consideration

This was linked to issues Jul 28, 2021
use_path_as_transaction_name has no effect #1924
Closed
spring cloud gateway transaction is empty #1934
Closed
@SylvainJuge
SylvainJuge merged commit ccb51be into elastic:master Jul 28, 2021
@SylvainJuge
SylvainJuge deleted the http-tx-priority-and-fallback branch July 28, 2021 11:24
v1v added a commit to v1v/apm-agent-java that referenced this pull request Aug 2, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/add-java-…
ed364ff 
…junit-support

* upstream/master:
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
v1v added a commit to v1v/apm-agent-java that referenced this pull request Aug 2, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/pipeline-…
178844e 
…for-windows-only

* upstream/master: (100 commits)
  [CI] Enable compatibility test matrix for unit tests (elastic#1915)
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
  Bump version.slf4j from 1.7.31 to 1.7.32 (elastic#1933)
  Add description about memory pool metrics to docs (elastic#1925)
  updated team membership check in action
  Add 1.25.0 to cloudfoundry index
  Update CHANGELOG.asciidoc (elastic#1929)
  fixed community label action
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release v1.25.0
  Prepare release 1.25.0 (elastic#1927)
  synchronize json schema specs (elastic#1926)
  added labeling of community issues and PRs
  added community labeler config
  ...
This was referenced Aug 9, 2021
Closed
Closed
@SylvainJuge SylvainJuge mentioned this pull request Aug 24, 2021
Closed
19 tasks
@felixbarny felixbarny mentioned this pull request Nov 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@eyalkoren eyalkoren eyalkoren approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

agent-java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

spring cloud gateway transaction is empty use_path_as_transaction_name has no effect

4 participants

@SylvainJuge @tobiasstadler @eyalkoren @elastic-apm-tech