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(lib): remove group separator from main user group #1119

Closed
wants to merge 1 commit into from

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Aug 23, 2019

This change now generates group tags according to @ppcano's proposal in #948:

{"type":"Point","data":{"time":"2019-08-23T16:00:05.35818107+02:00","value":0.166917,"tags":{"group":"mainGroup","method":"GET","name":"http://httpbin.org/get","proto":"HTTP/1.1","status":"200","url":"http://httpbin.org/get"}},"metric":"http_req_receiving"}
{"type":"Point","data":{"time":"2019-08-23T16:00:05.4684003+02:00","value":1,"tags":{"group":"mainGroup::nestedGroup","method":"GET","name":"http://httpbin.org/get","proto":"HTTP/1.1","status":"200","url":"http://httpbin.org/get"}},"metric":"http_reqs"}

This is a small change, but it seems backwards incompatible to me, in the sense that it will break user scripts that depended on the previous tag names. @na-- @mstoykov Thoughts?

Closes #948

@imiric imiric requested review from mstoykov and na-- August 23, 2019 14:57
@ppcano
Copy link
Contributor

ppcano commented Aug 23, 2019

Yes, I think this will break how the cloud service processes the group tag.

Out of the cloud service, I don't think many users are creating filters based on the group tag, but I may be wrong.

@imiric imiric requested a review from cuonglm August 27, 2019 13:54
@codecov-io
Copy link

Codecov Report

Merging #1119 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1119   +/-   ##
=======================================
  Coverage   73.28%   73.28%           
=======================================
  Files         141      141           
  Lines       10287    10287           
=======================================
  Hits         7539     7539           
  Misses       2304     2304           
  Partials      444      444
Impacted Files Coverage Δ
lib/models.go 94.52% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c202415...cf93bbb. Read the comment docs.

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Aug 29, 2019
@na--
Copy link
Member

na-- commented Aug 30, 2019

Sorry, since this is a minor breaking change, let's put this on the back-burner a bit, until we have some time to evaluate how it affects the UX in the cloud and in the end-of-test summary, as well as things like this issue: #949

@na--
Copy link
Member

na-- commented Jan 26, 2021

Closing because of #948 (comment), we decided to improve the issue from the other side to avoid major breaking changes

@na-- na-- closed this Jan 26, 2021
@na-- na-- deleted the feat/948-no-group-separator-on-main-group branch January 26, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to modify the group tag syntax.
5 participants