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

[Journeys] Set traceparent for Playwright #189800

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dgieselaar
Copy link
Member

Sets the traceparent for Playwright, so the trace from the test runner includes the trace events from the browser and Kibana server.

@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v8.16.0 labels Aug 2, 2024
@dgieselaar dgieselaar requested review from a team as code owners August 2, 2024 13:51
@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@dgieselaar
Copy link
Member Author

This needs elastic/apm-agent-rum-js#1521 to really work - currently the browser transaction becomes the root transaction and the parent of the backend transaction/span, and in the Journey setup, that means that you can no longer navigate the full trace from the perspective of the test runner.

@dgieselaar
Copy link
Member Author

I have verified elastic/apm-agent-rum-js#1521 works, and the initial page load is associated with the first step from the journey. However, when the page load transaction expires, and new managed transactions are started (e.g. based on an http request or a user interaction event), a new trace ID will be generated, and we can no longer see these events in the trace waterfall. I have reached out to the agent devs to see if there is a solution for that.

@dgieselaar dgieselaar requested a review from a team as a code owner August 27, 2024 10:07
@dgieselaar
Copy link
Member Author

Pushed a change that uses page.evaluate() to override trace.id and parent.id on every created Transaction. It's a private API so not sure if it's sustainable, will discuss with the agent folks.

@@ -45,7 +45,8 @@ export const getApmConfig = (requestPath: string) => {
...config,
pageLoadTraceId: traceId,
pageLoadSampled: sampled,
pageLoadSpanId: backendTransaction.ensureParentId(),
Copy link
Member

Choose a reason for hiding this comment

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

Q: should we keep this one and use the other ones only as fallbacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

pageLoadSpanId inverses the relationship - the RUM page-load transaction becomes the parent of the transaction on the backend. I still don't get what the use case is for this (or maybe I forgot), to me it makes more sense to have the server transaction to render the HTML be the parent of the page-load transaction. What are your thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I'm not familiar with why we did this and what ensureParentId() does. I'm mostly concerned about breaking the current behavior (I'm not sure if it's already broken 😅).

What you're saying makes a lot of sense to me. However, as I was trying to understand what this parameter does, I found this piece of documentation recommending the use of ensureParentId as the pageLoadSpanId on dynamically generated HTML.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless anyone is depending on the fact that the RUM transaction is the parent of the backend transaction (I honestly can't think of a use case for this), I think we can safely change this. However, I've reverted this change so we don't have to wait until elastic/apm-agent-rum-js#1521 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Both serve different purpose, For all APM agents (before Synthetics existed) RUM transaction is always considered the root.

  • So whenever the HTML is rendered from the Server, the Original Navigation timing span Requesting and Receiving the document will include the timing spent on the backend server which captures the time between requestStart and responseEnd. This is the part where the pageLoadSpanId helps where we can tie the frontend navigation with the backend HTML response generation.
  • Post Synthetics world, we need a new way to keep the RUM transaction as a child of the Synthetics one since browser is started from the synthetics side (Lots of details are in this issue - [Proposal] Crosslinking Synthetics with APM synthetics#265). This PR feat: add pageLoadParentId configuration apm-agent-rum-js#1521 solves this linking by creating a new option to override the RUM parent trace.

Hope this helps.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

@dmlemeshko
Copy link
Member

posting update for visibility:

After running journeys on CI, we still have an error and missing kibana server traces. Example for dashboard_listing_page journey
image

We are waiting for the RUM agent PR with required changes to be merged.

@@ -15,7 +15,7 @@ export const JOURNEY_APM_CONFIG = {
secretToken: APM_PUBLIC_TOKEN,
active: 'true',
contextPropagationOnly: 'false',
environment: process.env.CI ? 'ci' : 'development',
environment: process.env.ELASTIC_APM_ENVIRONMENT || (process.env.CI ? 'ci' : 'development'),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if its required, ENV variables overrides everything by default on the agent level? If this is for RUM agent, then yes its needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's for the RUM agent

return;
}
win.traceIdOverrideListenerAttached = true;
win.elasticApm!.observe('transaction:start', (tx) => {
Copy link
Member

Choose a reason for hiding this comment

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

there is a high chance this gets changed before transaction:end gets fired. Better would be to do it at that time.

@dgieselaar dgieselaar added v9.0.0 backport:version Backport to applied version labels v8.18.0 and removed v8.16.0 labels Jan 3, 2025
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants