[Execution Context] Update on URL change

[afharo]

Summary

Resolves #195778

This PR attempts to fix #195778 and https://github.com/elastic/observability-dev/issues/3776 by calling executionContext.set({ url }) whenever we identify the pathname has changed (using the history module for that).

Hopefully, this will remove the need of plugins explicitly calling the execution context to update the path when their internal Router applies any changes.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

Identify risks

• The solution is using executionContext.set instead of executionContext.clear. The difference is that any previous contextual information will be kept. When switching apps, we clear every previous context. However, when inside the same app, it didn't feel right to clear everything. Should it?

[elasticmachine]

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

• Click to trigger kibana-pull-request for this PR!
• Click to trigger kibana-deploy-project-from-pr for this PR!

[afharo]

/ci

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: 57712db

Failed CI Steps

• Check Types

History

• 💔 Build #252478 failed 1810ac5

cc @afharo

[Dosant]

@afharo, thank for looking into it!

Not sure I understand how this helps, I might be missing something here

I thought that we already reported the correct url from here:

kibana/packages/core/execution-context/core-execution-context-browser-internal/src/execution_context_service.ts

Lines 103 to 104 in b628770

	name: this.appId,
	url: window.location.pathname,

The problem with #195778 is that we don't report pageName that could be useful for segmentation in https://github.com/elastic/observability-dev/issues/3776

so, for example, in the url we'd get the actual URL with dynamic params: service/apm/transactions or service/frontend/transactions, but what would be useful is to have service/:serviceName/transactions which is how it was supposed to be reported in pageName if not the broken #195778

[afharo]

I thought that we already reported the correct url from here:

You are correct! The URL is correctly updated there, but the executionContext only updates it automatically when the app changes.

However, when the URL changes, we don't update it.

The problem with #195778 is that we don't report pageName that could be useful for segmentation in elastic/observability-dev#3776

so, for example, in the url we'd get the actual URL with dynamic params: service/apm/transactions or service/frontend/transactions, but what would be useful is to have service/:serviceName/transactions which is how it was supposed to be reported in pageName if not the broken #195778

That's a great point! My PR would only update the path on URL changes providing the full path (service/apm/transactions), but we may want more generic paths.

I wonder if this is a good fallback, but it still requires the ReactRouter to override this when necessary. WDYT?

[Dosant]

You are correct! The URL is correctly updated there, but the executionContext only updates it automatically when the app changes.
However, when the URL changes, we don't update it.

Aha, so

kibana/packages/core/execution-context/core-execution-context-browser-internal/src/execution_context_service.ts

Lines 101 to 104 in b628770

	return {
	type: 'application',
	name: this.appId,
	url: window.location.pathname,

is not called on every sent event? instead it is called only when executioncontext.set is called?
Then maybe instead of listening to history, we should always call getDefaultContext() to re-evaluate it. wdyt? then we don't need to listen to history. or it is more difficult?

[afharo]

we should always call getDefaultContext() to re-evaluate it

Sorry, what do you mean with "always"?

[Dosant]

Discussed offline. Forget previous questions, history.listen makes sense considering how the system is set up

[Dosant]

it might be that location.pathname didn't change, right? Maybe worth calling only when has changed?