Skip to content

[chore] prom translation rw2 add support for target_info #40493

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jmichalek132
Copy link
Contributor

Description

Adds support for adding target_info in the RW2 translation path.

Link to tracking issue

Fixes

Partially implements #33661 (when merging PR please don't close the tracing issue)

Testing

  • Added unit test
  • e2e run with prometheus

Target_info in prometheus.
image

@jmichalek132 jmichalek132 requested review from dashpole and a team as code owners June 4, 2025 18:53
@jmichalek132 jmichalek132 changed the title chore: prom translation rw2 add support for target_info [chore] prom translation rw2 add support for target_info Jun 4, 2025
@jmichalek132
Copy link
Contributor Author

Review please @dashpole @ArthurSens @ywwg @krajorama

@jmichalek132 jmichalek132 requested a review from ArthurSens June 5, 2025 15:32
@ywwg
Copy link

ywwg commented Jun 6, 2025

similar to #40494, this is feat, not a chore

Copy link

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial questions

// convert ns to ms
Timestamp: convertTimeStamp(timestamp),
}
converter.addSample(sample, labels, metadata{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason this is a pure function with the converter as a parameter instead of making this just a member of converter? I think that would make the function calls more clear:

c.AddResourceTargetInfoV2(resource, settings, mostRecentTimestamp)

name := prometheustranslator.TargetInfoMetricName
if len(settings.Namespace) > 0 {
// TODO what to do with this in case of full utf-8 support?
name = settings.Namespace + "_" + name
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have functions like "BuildCompliantMetricName" that take care of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants