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

Add tracing #1100

Merged
merged 26 commits into from
Dec 11, 2023
Merged

Add tracing #1100

merged 26 commits into from
Dec 11, 2023

Conversation

ka3de
Copy link
Collaborator

@ka3de ka3de commented Nov 14, 2023

What?

This PR uses the tracing instrumentation provided by k6 through the VU.State in order to generate traces that represent the inner workings of k6 browser.

Why?

Tracing has been chosen as the best suited method in order to build a timeline view of a k6 browser test, which can provide a more visual representation of a browser test along with the metrics associated for each action, such as specific WebVitals measurements per navigation, errors produced, etc.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Related: grafana/k6#3445

@ka3de ka3de added the feature A new feature label Nov 14, 2023
@ka3de ka3de self-assigned this Nov 14, 2023
@ka3de ka3de force-pushed the feat/tracing branch 7 times, most recently from 37a5123 to f635676 Compare November 17, 2023 07:20
@ka3de ka3de marked this pull request as ready for review November 17, 2023 07:36
@ka3de ka3de requested review from ankur22 and inancgumus November 17, 2023 07:36
common/trace.go Show resolved Hide resolved
@ka3de ka3de mentioned this pull request Nov 17, 2023
3 tasks
inancgumus
inancgumus previously approved these changes Nov 21, 2023
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Colossal work and nicely done 👏 I have some weak suggestions. Thanks!

browser/registry.go Outdated Show resolved Hide resolved
common/frame_session.go Outdated Show resolved Hide resolved
common/js/web_vital_init.js Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/screenshotter.go Outdated Show resolved Hide resolved
common/trace.go Show resolved Hide resolved
ankur22
ankur22 previously approved these changes Nov 29, 2023
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

This is great! Excited to see the results, and the next steps.

One question I have is why were only a subset of APIs instrumented?

common/frame_session.go Show resolved Hide resolved
common/frame_session.go Outdated Show resolved Hide resolved
common/trace.go Show resolved Hide resolved
@ka3de
Copy link
Collaborator Author

ka3de commented Dec 11, 2023

One question I have is why were only a subset of APIs instrumented?

I knew I have read this question somewhere but missed to answer it @ankur22.
The methods instrumented are the ones that were implemented in the PoC which was accepted by the working group, so these are the ones that I focused on. Instrumenting more exposed API methods should be straight forward now, so I would say let's merge this implementation first, and then we can evaluate if more methods are required for the next release. What do you think?

ankur22
ankur22 previously approved these changes Dec 11, 2023
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

inancgumus
inancgumus previously approved these changes Dec 11, 2023
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

🚀

This package provides high level tracing APIs tailored to k6 browser
needs, defining methods that allow to correlate async events with the
pages to which they belong to.
Adds a new traces registry which will hold the references for every
iteration root span and wrapping context. This span will be extended
for every instrumented method call during the iteration execution.
The traces registry has to be held per VU scope, as it stores traces
indexed per iteration, which are only unique per VU. Therefore keep it
under browser registry so we can use it to handle traces generation
based on iter events.
Because VU.State is nil when the browser registry is initialized, we
have to initialize the traces registry on the first VU iteration so we
can get access to the k6 TracerProvider. This is done within a sync.Once
so we ensure the traces registry is only initialized once per VU.
This also sets the trace context, wrapping the iteration's root span, as
the context for browser, which will be inherited by the other components
such as browser context, page, etc.
Only stop the traces registry if it has been previously initialized,
which won't happen for the initial VU.
When stopping, ensure any active live span is ended before exiting.
This is so the tracer can be retrieved by the other components which
inherit the browser context (BrowserContext, Page, etc) and use it to
generate spans that represent their actions.
Generate a span that represents a WebVital measurement.
@ka3de ka3de dismissed stale reviews from inancgumus and ankur22 via 25eebcf December 11, 2023 10:52
@ka3de ka3de force-pushed the feat/tracing branch 2 times, most recently from 25eebcf to f4cdfc8 Compare December 11, 2023 11:06
ka3de added 12 commits December 11, 2023 12:08
Defines a tracer interface with only the methods required for the common
package tracing. This also allows us to implement higher level helper
functions that will automatically retrieve the Tracer from a given
context and use it to generate a span, or return a noop span if no
tracer is found in the context, avoiding a possible NPE.
This functions will try to call the same method they implement from the
tracer contained in the given context. If the tracer is not found in the
context, they return a noop Span. This makes the code in the common
package cleaner and avoids dealing with possible NPEs. At the sime time
avoids having to mock the tracer in tests.
Allows to set a specific TracerProvider for the test VU implementation.
If not set, it defaults to NoopTracerProvider.
Reduces nested levels.
Modifies the browser registry constructor to accept a context that will
be used in order to build the browser context which will eventually be
inherited by the other browser components.
@ka3de
Copy link
Collaborator Author

ka3de commented Dec 11, 2023

Squashed fixup commits, rebased on top of main to resolve conflict on go.mod and fixed a couple of lint error that appeared after applying the fixup commits.

Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ka3de ka3de merged commit 3e936af into main Dec 11, 2023
17 checks passed
@ka3de ka3de deleted the feat/tracing branch December 11, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants