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

Improve detail in stdout exporter #436

Merged
merged 5 commits into from
Jan 24, 2020

Conversation

rebeccajae
Copy link
Contributor

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 in api/global/internal/meter_test.go which uses the stdout exporter. Since this adds a specified label to the counter, the new handling of the stdout exporter changes the output.

@rebeccajae rebeccajae marked this pull request as ready for review January 23, 2020 20:26
Copy link
Contributor

@rghetia rghetia left a 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?

@rebeccajae
Copy link
Contributor Author

@rghetia - I put the test in the api/global/internal/meter_test.go file. Although it's not grouped with the stdout exporter, the test case is similar to a pre-existing test case in that file. Additionally, since global state is stored in internal I couldn't modify it from the stdout_test.go file easily.

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.

@lizthegrey lizthegrey requested a review from rghetia January 23, 2020 23:39
@rghetia
Copy link
Contributor

rghetia commented Jan 23, 2020

@rghetia - I put the test in the api/global/internal/meter_test.go file. Although it's not grouped with the stdout exporter, the test case is similar to a pre-existing test case in that file. Additionally, since global state is stored in internal I couldn't modify it from the stdout_test.go file easily.

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())
}

@lizthegrey
Copy link
Member

I'd like @paivagustavo's eyes before we merge.

Copy link
Member

@paivagustavo paivagustavo left a 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 :)

@rghetia rghetia merged commit 20bb650 into open-telemetry:master Jan 24, 2020
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants