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

k6/execution: Add metadata JS API #3037

Merged
merged 4 commits into from
May 9, 2023
Merged

k6/execution: Add metadata JS API #3037

merged 4 commits into from
May 9, 2023

Conversation

mstoykov
Copy link
Contributor

A bare minimum API to set and get metadata set for the whole VU.

It is added on top of the vu property and on top of a new metrics one so. If k6/execution is imported as exec -
exec.vu.metrics.metadata["foo"] will get you the currently set metadata for foo the same way exec.vu.tags["bar"] will get you the value of the tag bar.

Setting metadata is the same.

It also adds exec.vu.metrics.tags which is the same as exec.vu.tags for consistency. While leaving the old one for backwards compatibility.

This also includes dropping the ability to list metadata keys and values from exec.vu.tags. This was likely left in by mistake.

This is a breaking change due to the fact that before that iter and vu could've been accessed this way but this will now need to be done through the new exec.vu.metrics.metadata.

Closes #2766

A bare minimum API to set and get metadata set for the whole VU.

It is added on top of the vu property and on top of a new `metrics` one
so. If `k6/execution` is imported as `exec` -
`exec.vu.metrics.metadata["foo"]` will get you the currently set
metadata for `foo` the same way `exec.vu.tags["bar"]` will get you the
value of the tag `bar`.

Setting metadata is the same.

It also adds `exec.vu.metrics.tags` which is the same as `exec.vu.tags`
for consistency. While leaving the old one for backwards compatibility.

This also includes dropping the ability to list metadata keys and values
from `exec.vu.tags`. This was likely left in by mistake.

This is a breaking change due to the fact that before that `iter` and
`vu` could've been accessed this way but this will now need to be done
through the new `exec.vu.metrics.metadata`.

Closes #2766
@mstoykov mstoykov added this to the v0.45.0 milestone Apr 26, 2023
@github-actions github-actions bot requested review from codebien and na-- April 26, 2023 12:52
@na-- na-- removed their request for review April 26, 2023 14:29
@mstoykov mstoykov requested review from olegbespalov and imiric and removed request for olegbespalov April 28, 2023 09:22
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved
js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved
js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved
@mstoykov
Copy link
Contributor Author

note: I will just squash at the end

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2023

Codecov Report

Merging #3037 (95a2c7b) into master (ec094ce) will increase coverage by 0.00%.
The diff coverage is 78.84%.

❗ Current head 95a2c7b differs from pull request most recent head f011c23. Consider uploading reports for the commit f011c23 to get more accurate results

@@           Coverage Diff           @@
##           master    #3037   +/-   ##
=======================================
  Coverage   77.04%   77.05%           
=======================================
  Files         229      229           
  Lines       17065    17107   +42     
=======================================
+ Hits        13147    13181   +34     
- Misses       3077     3084    +7     
- Partials      841      842    +1     
Flag Coverage Δ
ubuntu 77.05% <78.84%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/consts/consts.go 78.57% <ø> (ø)
lib/executor/base_config.go 86.48% <ø> (ø)
lib/options.go 89.62% <ø> (ø)
js/modules/k6/execution/execution.go 83.49% <73.68%> (-0.13%) ⬇️
js/common/tags.go 93.75% <92.85%> (+4.27%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, just minor suggestions.

js/common/tags.go Outdated Show resolved Hide resolved
js/common/tags.go Outdated Show resolved Hide resolved
Comment on lines 222 to 225
})
err = o.Set("tags", tagsDynamicObject)

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
err = o.Set("tags", tagsDynamicObject)
if err != nil {
})
// This is kept for backwards compatibility reasons, but should be deprecated,
// since tags are also accessible via vu.metrics.tags.
err = o.Set("tags", tagsDynamicObject)
if err != nil {

Also, should we add a deprecation warning when this is first used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to add an issue to discuss if we even want to deprecate it 🤷

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved
js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved
js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved
@mstoykov mstoykov requested review from codebien and imiric May 9, 2023 14:25
@mstoykov mstoykov merged commit 9e3359b into master May 9, 2023
@mstoykov mstoykov deleted the metadataJSAPI branch May 9, 2023 15:03
@mstoykov mstoykov added the documentation-needed A PR which will need a separate PR for documentation label May 15, 2023
@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 8, 2023

documented in grafana/k6-docs#1207

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API for user-specified high-cardinality metrics metadata
4 participants