receiver/prometheus/internal: augment metricFamilyPdata from big PR#5185
receiver/prometheus/internal: augment metricFamilyPdata from big PR#5185odeke-em wants to merge 1 commit into
Conversation
|
Kindly cc-ing @Aneurysm9 @alolita @dashpole @tigrannajaryan to help with a +1 |
dashpole
left a comment
dashpole
left a comment
There was a problem hiding this comment.
Might be worth updating the description to say what this does.
It adds open-telemetry/opentelemetry-collector#3116 to the otlp version.
It copies the sortPoints method over verbatim.
It sets type and name on pdata metrics.
This change updates internal code and is meant to alleviate the massive PR #5184 which is our eventual end-goal. No need for tests because the code path isn't used so we have a license to update it towards the end goal. Particularly it: * adds open-telemetry/opentelemetry-collector#3116 to the otlp version. * copies the sortPoints method over verbatim. * sets DataType and Name on pdata metrics. Updates PR #5184
odeke-em
force-pushed
the
recceiver-prometheus-update-internal-OTLP
branch
from
d404249 to
55560d6
Compare
Done, thank you @dashpole! Thank you @alolita @dashpole @cuonglm for the reviews and approvals! Kindly cc-ing @bogdandrutu @Aneurysm9 to help with a merge. |
|
|
||
| mtype := convToPdataMetricType(metadata.Type) | ||
| if mtype == pdata.MetricDataTypeNone { | ||
| logger.Debug(fmt.Sprintf("Invalid metric : %s %+v", metricName, metadata)) |
There was a problem hiding this comment.
MetricName and metadata should fields not a dynamic string to allow for easier searching in logging systems.
There was a problem hiding this comment.
I also question the value of having the log statement here?
There was a problem hiding this comment.
It's in the original code
this code is a port. @MovieStoreGuy I appreciate the enthusiasm but the context here is that I am porting over code, which requires careful parity changes; you can file bugs for improvements but these PRs am sending are for porting over code.
There was a problem hiding this comment.
I've opened a followup issue here #5720, can this comment be resolved @MovieStoreGuy?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@odeke-em please resolve the conflict |
|
@odeke-em any plans to update this PR? Otherwise we should close it. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
…telemetry#5185) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
9 participants
This change updates internal code and is meant to alleviate the
massive PR #5184 which is our eventual end-goal. No need for tests
because the code path isn't used so we have a license to update
it towards the end goal.
Particularly it:
Updates PR #5184