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

Added support for setting the framework using the public api (#1908) #1909

Merged
merged 8 commits into from
Jul 13, 2021

Conversation

tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented Jul 10, 2021

What does this PR do?

Fixes #1908

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

@apmmachine
Copy link
Contributor

apmmachine commented Jul 10, 2021

💚 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-13T09:33:00.958+0000

  • Duration: 53 min 19 sec

  • Commit: 8dec8e5

Test stats 🧪

Test Results
Failed 0
Passed 2349
Skipped 19
Total 2368

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 2349
Skipped 19
Total 2368

@eyalkoren
Copy link
Contributor

Thanks for this PR @tobiasstadler !

Some things are missing:

  1. document the added API to docs/public-api.asciidoc
  2. providing higher precedence to user-set values, both is set before AND after the automatic setting
  3. the implementation of 2 should include the ability to set null or empty string in order to override the automatically-set value
  4. testing 2 and 3 - before/after automatic, with null/empty and valid value
  5. extend the javadoc and asciidoc to specify the above

You can see examples for all of those in #1788

@tobiasstadler
Copy link
Contributor Author

@eyalkoren I did the requested changes

The javadoc/documentation states

Override the name of the framework for the current transaction.
For supported frameworks,
the framework name is determined automatically,
and can be overridden using this function.

In my opinion this clearly says that a user set value has a higher precedence that an automatic set value (by a plugin). Do you agree?

@eyalkoren
Copy link
Contributor

In my opinion this clearly says that a user set value has a higher precedence that an automatic set value (by a plugin). Do you agree?

Absolutely agree, only a word that setting null or empty value will make the agent omit this field, just as a clarification

@eyalkoren
Copy link
Contributor

run elasticsearch-ci/docs

@eyalkoren eyalkoren merged commit 6004245 into elastic:master Jul 13, 2021
@tobiasstadler tobiasstadler deleted the fix-1908 branch July 13, 2021 13:04
@tobiasstadler
Copy link
Contributor Author

Thank You!

@eyalkoren
Copy link
Contributor

Thank YOU! 😄

v1v added a commit to v1v/apm-agent-java that referenced this pull request Jul 27, 2021
* master:
  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
  Upgrade Byte Buddy to 1.11.8 (elastic#1923)
  Bump json-schema-validator from 1.0.56 to 1.0.57 (elastic#1916)
  Bump log4j2-ecs-layout from 1.0.1 to 1.1.0 (elastic#1917)
  Added support for setting the framework using the public api (elastic#1908) (elastic#1909)
  Add Javalin plugin (elastic#1822)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for setting the framework using the public api
3 participants