-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve detail in stdout exporter #436
Improve detail in stdout exporter #436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Can you please add a test for empty label?
@rghetia - I put the test in the I'm not sure if this is the best place for that test, and I felt that just trying to use non-colliding meter names could lead to confusion if the tests grow and re-use meter names. |
test in stdout_test.go is preferable. For example (based on TestStdoutCounter),. func TestStdoutCounterWithUnspecifiedKeys(t *testing.T) {
fix := newFixture(t, stdout.Config{})
checkpointSet := test.NewCheckpointSet(sdk.NewDefaultLabelEncoder())
keys := []core.Key{key.New("E")}
desc := export.NewDescriptor("test.name", export.CounterKind, keys, "", "", core.Int64NumberKind, false)
cagg := counter.New()
aggtest.CheckedUpdate(fix.t, cagg, core.NewInt64Number(123), desc)
cagg.Checkpoint(fix.ctx, desc)
checkpointSet.Add(desc, cagg, key.String("A", "B"), key.String("C", "D"))
fix.Export(checkpointSet)
require.Equal(t, `{"updates":[{"name":"test.name{A=B,C=D,E}","sum":123}]}`, fix.Output())
} |
I'd like @paivagustavo's eyes before we merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with the discussions that you raised on this issue and with your solution, @rebeccajae. Thanks :)
This addresses the issue raised in #425 regarding the level of detail provided by the
stdout
exporter.As per the comments on that issue, I've opted to use just key names to represent unspecified keys. This seemed to be more well-received than putting them in a separate field.
In addition, this adjusts the test
TestDefaultSDK
inapi/global/internal/meter_test.go
which uses thestdout
exporter. Since this adds a specified label to the counter, the new handling of thestdout
exporter changes the output.