-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
plugins/parsers/json: Use same timestamp for all objects in arrays #7412
Conversation
All objects arrive at the same time so they should have the same timestamp
Looks great, thanks so much for the contribution! |
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 great, do you think you can add a unit test too?
I did run
I figured the code coverage would be unchanged due to the nature of the change. Was there some specific test you would like to see? |
Just something to check and document the behavior: parse a document that creates two metrics and ensure that the timestamp is the same between them. The document could be something very simple like: [{"value": 1}, {"value":2}] |
Okay, I see what you are saying. It looks like the unit test is in |
You would want to add the test to func TestShareTimestamp(t *testing.T) {
parser, err := New(
&Config{
MetricName: "json",
},
)
require.NoError(t, err)
actual, err := parser.Parse([]byte(`[{"value": 1}, {"value":2}]`))
require.NoError(t, err)
// check that all metrics have the same timestamp
} |
Thanks a lot for your help. I have added the test. |
(cherry picked from commit e1b2ebe)
I am new to this, so please let me know if this is not appropriate.
My data is sent as a single JSON packet but the resulting measurements generated from the JSON array all had different timestamps. This made it difficult to work with the data. Because all objects in the JSON array arrive at the same time they should have the same timestamp. With the same timestamp the data has the correlation that it should.
Required for all PRs: