Skip to content

renamed span_frames_min_duration to span_stack_trace_min_duration. Ch… #2220

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Nov 9, 2021
Merged

renamed span_frames_min_duration to span_stack_trace_min_duration. Ch… #2220

merged 18 commits into from
Nov 9, 2021

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Oct 30, 2021 •
edited by zube bot
Loading

…anged logic when reporting.

What does this PR do?

closes #2206

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 Oct 30, 2021
@ghost
Copy link

ghost commented Oct 30, 2021 •
edited by ghost
Loading

💚 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-11-09T08:43:07.135+0000

  • Duration: 67 min 13 sec

  • Commit: d53c77f

Test stats 🧪

Test Results
Failed 0
Passed 2519
Skipped 19
Total 2538

💚 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 test.

  • run compatibility tests : Run the JDK Compatibility test.

  • run integration tests : Run the APM-ITs.
felixbarny
felixbarny requested changes Nov 2, 2021
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The semantics for span_frames_min_duration change in this PR but should stay the same. Make the current config option internal and create a new one for span_stack_trace_min_duration. Then, consider both values when making the decision and give precedence to the new value.
@videnkz
Copy link
Contributor Author

videnkz commented Nov 5, 2021

Thanks for the PR!

The semantics for span_frames_min_duration change in this PR but should stay the same. Make the current config option internal and create a new one for span_stack_trace_min_duration. Then, consider both values when making the decision and give precedence to the new value.

Hi @felixbarny ,
Do I need to leave the documentation about span_frames_min_duration in configuration.asciidoc?
@felixbarny
Copy link
Member

felixbarny commented Nov 5, 2021

Do I need to leave the documentation about span_frames_min_duration in configuration.asciidoc?

No, I think it's fine if the old option doesn't show up in the docs anymore.
@videnkz videnkz force-pushed the stack_trace_span_min_duration branch from fa33808 to 5173d3c Compare November 5, 2021 10:58
videnkz and others added 3 commits November 5, 2021 18:02
@videnkz
updated logic and tests
8b656ec
@videnkz @felixbarny
Update apm-agent-core/src/main/java/co/elastic/apm/agent/impl/stacktr…
2bca395 
…ace/StacktraceConfiguration.java deleted aliasKey Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@videnkz
updated changelog
7ba6924
felixbarny
felixbarny approved these changes Nov 5, 2021
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

LGTM after applying the suggestions. Thanks!
videnkz and others added 3 commits November 5, 2021 19:51
@videnkz @felixbarny
Update apm-agent-core/src/main/java/co/elastic/apm/agent/impl/stacktr…
5ec2b50 
…ace/StacktraceConfiguration.java Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@videnkz @felixbarny
Update CHANGELOG.asciidoc
c142e76 
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@videnkz @felixbarny
Update apm-agent-core/src/main/java/co/elastic/apm/agent/impl/Elastic…
baf4292 
…ApmTracer.java Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@felixbarny
Copy link
Member

felixbarny commented Nov 8, 2021

run elasticsearch-ci/docs
@felixbarny felixbarny enabled auto-merge (squash) November 8, 2021 12:21
@felixbarny
Copy link
Member

felixbarny commented Nov 8, 2021

There's a remaining reference to config-span-frames-min-duration that makes the docs build fail.
auto-merge was automatically disabled November 8, 2021 17:03

Head branch was pushed to by a user without write access

@felixbarny
Copy link
Member

felixbarny commented Nov 8, 2021

/test
run elasticsearch-ci/docs
@felixbarny
Copy link
Member

felixbarny commented Nov 9, 2021

@kananindzya could you re-generate the config docs?

Tests / Unit Tests / testGeneratedConfigurationDocsAreUpToDate – co.elastic.apm.agent.configuration.ConfigurationExporterTest The rendered configuration documentation (/docs/configuration.asciidoc) is not up-to-date.
@videnkz
Copy link
Contributor Author

videnkz commented Nov 9, 2021

@kananindzya could you re-generate the config docs?

Tests / Unit Tests / testGeneratedConfigurationDocsAreUpToDate – co.elastic.apm.agent.configuration.ConfigurationExporterTest The rendered configuration documentation (/docs/configuration.asciidoc) is not up-to-date.

Sorry, there are left reference from CoreConfiguration. I fixed this. And now ./mvnw clean test -pl elastic-apm-agent -am -DfailIfNoTests=false -Dtest=ConfigurationExporterTest works succesfully.
@eyalkoren
Copy link
Contributor

eyalkoren commented Nov 9, 2021

run elasticsearch-ci/docs
@jackshirazi jackshirazi merged commit efe97c9 into elastic:master Nov 9, 2021
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

4 participants

@videnkz @felixbarny @eyalkoren @jackshirazi