-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[processor/transform] Wire up metrics processing #10100
[processor/transform] Wire up metrics processing #10100
Conversation
/cc @anuraaga |
transform: | ||
metrics: | ||
queries: | ||
- set(body, "bear") where attributes["http.path"] == "/animal" |
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.
This seems like it will fail on body
instead of the unknown function IIUC
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.
Ya I missed this in the copy and paste. Will fix.
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.
Fixed. Now properly fails with "processor "transform" has invalid configuration: undefined function not_a_function"
@@ -1622,3 +1622,7 @@ func createNewTelemetry() (pmetric.ExemplarSlice, pcommon.Map, pcommon.Value, pc | |||
func strp(s string) *string { | |||
return &s | |||
} | |||
|
|||
func intp(i int64) *int64 { |
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.
Is this used?
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.
Ya its used in metrics/functions_test.go
. It is defined in the traces_test
and logs_test
file as well despite being used different places.
rmetrics := td.ResourceMetrics().At(i) | ||
ctx.resource = rmetrics.Resource() | ||
for j := 0; j < rmetrics.ScopeMetrics().Len(); j++ { | ||
il := rmetrics.ScopeMetrics().At(j).Scope() |
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.
What's the value to calling this out separate from the assignment to ctx.il?
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.
None, it's a result of traces and logs doing it that way. I can clean it up.
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.
Cleaned up
for k := 0; k < metrics.Len(); k++ { | ||
metric := metrics.At(k) | ||
ctx.metric = metric | ||
switch ctx.metric.DataType() { |
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.
Inconsistent, the switch is using ctx.metric but all of the cases are using metric.Type().DataPoints. My opinion would be just assign directly ctx.metric = metrics.At(k)
and only use ctx.metric, but I think using metric for the switch would be fine too, I just think it should be consistent within the scope of the switch statement for clarity.
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.
Yes i will clean up the assignments/uses
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.
Cleaned up
Approving because Anuraag approved it :) |
lol! Lots of cleanup still to do but want to get it in alpha while the cleanup happens so people can give feedback |
* Started wiring up metrics * Started messing with tests * Add NumberDataPoint function tests * Finished metric function tests * Added processor tests * Added another processor test * change DataType Getter to string * Update readme * Update config and factory tests * Add more test data * Updated readme * clean up * Update readme * ran make gotidy * fix test data * cleanup processors * Remove ability to set data type Co-authored-by: Bogdan Drutu <[email protected]>
* Started wiring up metrics * Started messing with tests * Add NumberDataPoint function tests * Finished metric function tests * Added processor tests * Added another processor test * change DataType Getter to string * Update readme * Update config and factory tests * Add more test data * Updated readme * clean up * Update readme * ran make gotidy * fix test data * cleanup processors * Remove ability to set data type Co-authored-by: Bogdan Drutu <[email protected]>
Description:
Wires the metrics data model into the transform processor
Link to tracking Issue:
#8252
Testing:
Added new unit tests and updated existing
Documentation:
Updated README