Skip to content

Conversation

@tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented May 2, 2022

What does this PR do?

Fixes #2597

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

@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels May 2, 2022
@github-actions
Copy link

github-actions bot commented May 2, 2022

👋 @tobiasstadler Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

@ghost
Copy link

ghost commented May 2, 2022

💚 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: 2022-05-02T13:37:08.809+0000

  • Duration: 42 min 40 sec

Test stats 🧪

Test Results
Failed 0
Passed 2851
Skipped 20
Total 2871

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Thanks!
See the NOTE in #2597 - your new code is currently not tested because we set the service.version through additional fileds.
Part of this PR should be to replace the service.version in theses custom fields with another field and test that both custom fields are added.
The validation already tests that service.version is set, so existing tests should still pass, but my JUL ECS-reformatting PR would fail, as it should, until I fix it.

@tobiasstadler
Copy link
Contributor Author

Thanks!
See the NOTE in #2597 - your new code is currently not tested because we set the service.version through additional fileds.

Done

Part of this PR should be to replace the service.version in theses custom fields with another field and test that both custom fields are added.

Done

@eyalkoren eyalkoren requested a review from felixbarny May 2, 2022 11:51
Comment on lines 449 to 450
* This may mismatch automatically-discovered service version (if not configured). However, we only set it
* once when configuring our appender, so we can have only one service version.
Copy link
Member

Choose a reason for hiding this comment

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

This is because the auto-detection for different web apps happens after their logging setup is initialized, right?

[minor, feel free to ignore] I think the auto-detected version from a standalone jar (META-INF/MANIFEST.mf) would still be applied, right? Therefore, the variable configuredServiceVersion may be misleading as it could also be auto-discovered. Maybe rename to globalServiceVersion. But I see the same also applies to configuredServiceName so probably out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree. IIRC, it was really only the configured value when added and changed later to globally-auto-detected.
I'll change.

@eyalkoren
Copy link
Contributor

run elasticsearch-ci/docs

@eyalkoren eyalkoren enabled auto-merge (squash) May 2, 2022 13:38
@eyalkoren eyalkoren merged commit 56f986c into elastic:main May 2, 2022
@tobiasstadler
Copy link
Contributor Author

Thank You!

@tobiasstadler tobiasstadler deleted the fix-2597 branch May 3, 2022 05:37
@SylvainJuge SylvainJuge added await-release Mark issues that depend on next release, or PRs that are planned to be included and removed await-release Mark issues that depend on next release, or PRs that are planned to be included labels May 5, 2022
videnkz pushed a commit to videnkz/apm-agent-java that referenced this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-java community Issues and PRs created by the community triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set service.version in ECS-reformatting

4 participants