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

Implement default type fallback #2525

Merged
merged 10 commits into from Apr 4, 2022

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Mar 18, 2022

What does this PR do?

Implements elastic/apm#602

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

@apmmachine
Copy link
Collaborator

apmmachine commented Mar 18, 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-04-04T08:26:05.554+0000

  • Duration: 53 min 19 sec

Test stats 🧪

Test Results
Failed 0
Passed 2843
Skipped 20
Total 2863

💚 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!)

@SylvainJuge SylvainJuge marked this pull request as ready for review March 21, 2022 10:13
@github-actions
Copy link

/test

Comment on lines 678 to 683
protected String normalizeType(@Nullable String type) {
if (type == null || type.isEmpty()) {
return "custom";
}
return type;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] proposal - do that in a base implementation for beforeEnd() instead and call super.beforeEnd() in children

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest to do it without any sub-classing: by moving the type attribute directly into AbstractSpan, while the field value has different semantics in Transaction and Span this makes sense and allows to keep this normalization private in AbstractSpan.

I'll push an implementation of this in this PR, let me know if that would be a good fit here.

Copy link
Contributor

@eyalkoren eyalkoren Mar 21, 2022

Choose a reason for hiding this comment

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

Not sure. Why was it in the subclass in the first place? Is it because of the volatility requirement in case of transactions and not in case of spans (not sure why)? Maybe there was a reason for this separation.
I assumed it was in the base class when proposing that

Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't know why it was like this in the first place, doing some git archaeology it has been like this since 2018.
Maybe @felixbarny remember something about having two distinct fields and one of them being volatile, as this is not urgent we can definitely discuss this during next weekly meeting.

@SylvainJuge SylvainJuge enabled auto-merge (squash) March 28, 2022 14:49
@SylvainJuge SylvainJuge merged commit 72f88e9 into elastic:main Apr 4, 2022
APM-Agents (OLD) automation moved this from In Progress to Done Apr 4, 2022
@SylvainJuge SylvainJuge deleted the custom-when-type-empty branch April 4, 2022 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants