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

[APM] Diplay logs only matching trace.id query #136927

Merged
merged 5 commits into from
Aug 1, 2022

Conversation

kpatticha
Copy link
Contributor

@kpatticha kpatticha commented Jul 22, 2022

Summary

closes: #126342

related

#85859 (comment)

@kpatticha kpatticha requested a review from a team as a code owner July 22, 2022 08:19
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 22, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@dgieselaar
Copy link
Member

@kpatticha I assumed the wider matching was intentional, maybe to also display non-ECS compatible log messages. It's also how we link to the Logs UI. Did we run this by @alex-fedotyev? I think he is the in-house APM logs expert 😄

Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

Doesn't it make sense to also change the link in

filter: `trace.id:"${transaction.trace.id}" OR "${transaction.trace.id}"`,
?

@kpatticha
Copy link
Contributor Author

@kpatticha I assumed the wider matching was intentional, maybe to also display non-ECS compatible log messages. It's also how we link to the Logs UI. Did we run this by @alex-fedotyev? I think he is the in-house APM logs expert 😄

No Cyrille opened a bug for this. .
Oliver also pointed this in his PR when he worked on adding logs tab but couldn't find if it was intertional. My understading is we tried to follow the same as here query .

@alex-fedotyev what do you think about this change?

@sorenlouv
Copy link
Member

sorenlouv commented Jul 25, 2022

I assumed the wider matching was intentional, maybe to also display non-ECS compatible log messages

I originally suggested the wider matching in #79995. I can't remember why and cannot find any discussion of this so I suggest to go ahead with this change (as long as @alex-fedotyev don't have any objections).

@kpatticha kpatticha added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.4.0 labels Jul 29, 2022
@kpatticha kpatticha enabled auto-merge (squash) August 1, 2022 09:02
@kpatticha kpatticha merged commit 1e56f9b into elastic:main Aug 1, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.0MB 3.0MB +42.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 1, 2022
* [APM] Diplay logs only matching `trace.id` query

* Fix log stream query

* Update unit tests

(cherry picked from commit 1e56f9b)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Aug 1, 2022
* [APM] Diplay logs only matching `trace.id` query

* Fix log stream query

* Update unit tests

(cherry picked from commit 1e56f9b)

Co-authored-by: Katerina Patticha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:APM All issues that need APM UI Team support v8.4.0 v8.5.0
Projects
None yet
7 participants