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

feat: add pageLoadParentId configuration #1187

Closed
wants to merge 3 commits into from
Closed

feat: add pageLoadParentId configuration #1187

wants to merge 3 commits into from

Conversation

devcorpio
Copy link
Contributor

@devcorpio devcorpio commented Mar 16, 2022

Context

One of the requirements for Crosslinking Synthetics with APM issue was to allow the RUM agent the creation of the page-load transaction as child of the one created in Synthetics.

The main goal of that is to provide visibility into how the synthetic journeys are executed and what actions are happening inside every step. This image extracted from the issue linked above it's a great example.

Summary

We expose a new agent configuration option named pageLoadParentId whose value will be the id of the transaction that we want to establish as parent of the page-load transaction. Before this PR page-load transaction was always treated as the root transaction, now it will be possible to set a parent if needed thanks to this new config.

@apmmachine
Copy link
Contributor

apmmachine commented Mar 16, 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-03-23T14:03:17.246+0000

  • Duration: 70 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 774
Skipped 8
Total 782

🤖 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 elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@devcorpio devcorpio linked an issue Mar 21, 2022 that may be closed by this pull request
@devcorpio devcorpio changed the title wip: add pageLoadParentId configuration feat: add pageLoadParentId configuration Mar 22, 2022
@devcorpio devcorpio marked this pull request as ready for review March 22, 2022 10:07
@devcorpio
Copy link
Contributor Author

/test

@apmmachine
Copy link
Contributor

📦 Bundlesize report

Filename Size(bundled) Size(gzip) Diff(gzip)
elastic-apm-opentracing.umd.min.js 66.1 KiB 21.0 KiB ⚠️ 29 Bytes
elastic-apm-rum.umd.min.js 60.0 KiB 19.4 KiB ⚠️ 29 Bytes

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Thanks @devcorpio Changes LGTM, But we need to do few more things

  1. Add the limit check to truncation logic that should truncate if the field is past 1024 bytes-
  2. Document the pageloadParentId in the config option along with other trace options- https://github.com/elastic/apm-agent-rum-js/blob/main/docs/configuration.asciidoc#pageloadtraceid

I would also like to test with the Synthetics APM PR before we close this down. Thanks!

@devcorpio
Copy link
Contributor Author

devcorpio commented Mar 23, 2022

Thanks @devcorpio Changes LGTM, But we need to do few more things

  1. Add the limit check to truncation logic that should truncate if the field is past 1024 bytes-
  2. Document the pageloadParentId in the config option along with other trace options- https://github.com/elastic/apm-agent-rum-js/blob/main/docs/configuration.asciidoc#pageloadtraceid

I would also like to test with the Synthetics APM PR before we close this down. Thanks!

The truncation logic it's already working. The function truncate default limit is 1024 as we can see here.

Something that caught my attention is that if I replace parent_id: true with parent_id: [KEYWORD_LIMIT, true] and the value for parent_id is empty the truncate logic will set the N/A value which in my opinion feels a bit weird value for a parent_id.

One way to avoid that is setting parent_id: [KEYWORD_LIMIT, false] but seeing that the value was already being truncated, I would advocate for letting the config as it is. What do you think?

@vigneshshanmugam
Copy link
Member

One way to avoid that is setting parent_id: [KEYWORD_LIMIT, false] but seeing that the value was already being truncated, I would advocate for letting the config as it is. What do you think?

No, we dont need to set N/A and that flag exists only when we cannot send a empty string to the APM server which are marked as required: true in the server spec.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

LGTM

I will try to test with Synthetics package before we do a release of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apm-agent-rum does not set the custom transaction as root
3 participants