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

perf(rum-core): avoid url parsing on resource timing entries #174

Merged
merged 3 commits into from Feb 22, 2019

Conversation

vigneshshanmugam
Copy link
Member

  • Resource timing entries name have fully resolved URL as part of the SPEC and we dont really need to parse them again based on the window base URL.

  • The context information is sent only when the Entry Name is different from Span name -> Happens when the resource contains query strings in URL. small optimisation for reducing the payload to APM server.

Copy link
Contributor

@hmdhk hmdhk left a comment

Choose a reason for hiding this comment

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

@vigneshshanmugam
Copy link
Member Author

@jahtalab Do you need me to change anything? Or shall i rebase and we can merge this?

/**
* Add context information when the span name and entry name are different
*/
if (spanName !== name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vigneshshanmugam , I think we should consider this a breaking change since we're removing a field that users might have relied on!

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 am not sure here, I don't have any numbers on how much users search for context in kibana search UI. What we can do is to split this change and do in separate PR and not combine with this performance optimisation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think generally we should keep in mind how each change affects the users, since we're open source we might never know some of the use-cases!

Yeah, let's split to avoid having multiple changes in the same PR.

@hmdhk hmdhk merged commit 54ea6b9 into elastic:master Feb 22, 2019
@vigneshshanmugam vigneshshanmugam deleted the avoid-url-parse branch February 22, 2019 14:50
David-Development pushed a commit to David-Development/apm-agent-rum-js that referenced this pull request Oct 20, 2021
…#174)

* perf(rum-core): avoid url parsing on resource timing entries

* filter null values

* add context by defauly
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.

None yet

2 participants