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.

Sign up for GitHub

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

Jump to bottom

receiver/prometheusreceiver: add option to fallback to collector starttime #36365

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

receiver/prometheusreceiver: add option to fallback to collector starttime #36365

wants to merge 5 commits into from

Conversation

ridwanmsharif
Copy link
Contributor

Description

This change adds an option to the metric adjuster to use an approximation of the collector starttime as a fallback for the start time of scraped cumulative metrics. This is useful when no start time is found and when the collector starts up alongside its targets (like in serverless environments or sidecar approaches).

Link to tracking issue

Fixes #36364

Testing

Added unit test for this config option

Documentation

Config option added to the README.

@ridwanmsharif ridwanmsharif requested review from dashpole and a team as code owners November 14, 2024 02:37
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Nov 14, 2024
@github-actions github-actions bot requested a review from Aneurysm9 November 14, 2024 02:38
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch 3 times, most recently from 05b85e8 to d67a526 Compare November 14, 2024 15:15
@ArthurSens
Copy link
Member

The code itself looks correct!

To be completely honest, I'm pretty new to this component and haven't used it myself. I noticed we have waaaay too many fallbacks for Created Timestamps; that looks weird 🤔.

Do I understand correctly that the flow is like this:

  • If metric StartTimeUnixNano is set, use that to populate the Created Timestamp from the prometheus/client_golang SDK.
  • If StartTimeUnixNano is not set, get it from another metric called process_start_time_seconds (where does that come from?)
  • Finally, if we still don't have a timestamp, we use the collector start time.

Did I get the flow correctly?

It makes me wonder, when would an OpenTelemetry metric not have StartTimeUnixNano not set? I understand this is not a required field in the spec, but maybe we could work on making it required instead?

@dashpole dashpole added the enhancement New feature or request label Nov 21, 2024
@Aneurysm9
Copy link
Member

It makes me wonder, when would an OpenTelemetry metric not have StartTimeUnixNano not set? I understand this is not a required field in the spec, but maybe we could work on making it required instead?

This is the prometheus receiver, so the concern here is prometheus metrics not having a start time and we want to ensure that the pdata metrics produced by the receiver do have a start time. The flow you described appears correct, though it is in the context of populating StartTimeUnixNano instead of looking to it for a value.

@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 10, 2024
@dashpole dashpole removed the Stale label Dec 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 25, 2024
@dashpole dashpole removed the Stale label Jan 6, 2025
@ridwanmsharif
Copy link
Contributor Author

Sorry I let this get stale, I was ooo for the past couple months but am getting back to this today. I'll make the change to set the start time in an init function, that feels most appropriate here

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch 4 times, most recently from 5e0b231 to 2f6bba4 Compare January 14, 2025 21:32
dashpole
dashpole approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Can you add a feature gate around this (alpha) so this is easier for us to remove later if we move it to the metric start time processor?

@dashpole dashpole self-requested a review January 16, 2025 19:10
@dashpole
Copy link
Contributor

(accidental approval)

@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch from 2f6bba4 to 4fc4202 Compare January 20, 2025 19:03
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch 4 times, most recently from 86ee36f to 80e5d9d Compare January 20, 2025 22:07
@ridwanmsharif
Copy link
Contributor Author

Can you add a feature gate around this (alpha) so this is easier for us to remove later if we move it to the metric start time processor?

Done. Makes sense to me

@ridwanmsharif
receiver/prometheusreceiver: add option to fallback to collector star…
a472748 
…ttime

This change adds an option to the metric adjuster to use an
approximation of the collector starttime as a fallback for the start
time of scraped cumulative metrics. This is useful when no start time is
found and when the collector starts up alongside its targets (like in
serverless environments or sidecar approaches).

Signed-off-by: Ridwan Sharif <[email protected]>
@ridwanmsharif
prometheusreceiver: approximate collector start time in an init function
955a6ad
@ridwanmsharif
prometheusreceiver: use gopsutil to gather collector start time on co…
c783a95 
…mponent initialization
@ridwanmsharif
prometheusreceiver: change start time fallback from a config knob to …
28579d1 
…a featuregate

This change creates an Alpha feature gate for this functionality since
this can be replaced when
open-telemetry#37186
is implemented.
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch 3 times, most recently from 69d33dc to 6cc71ee Compare January 23, 2025 19:11
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/starttime-fallback branch from 6cc71ee to d28d7d2 Compare January 23, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

@dehaansa dehaansa dehaansa approved these changes

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@chrroberts-pure chrroberts-pure Awaiting requested review from chrroberts-pure

@dgoscn dgoscn Awaiting requested review from dgoscn

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@ArthurSens ArthurSens Awaiting requested review from ArthurSens

Assignees

@bogdandrutu bogdandrutu

Labels
enhancement New feature or request receiver/prometheus Prometheus receiver
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

receiver/promettheusreceiver: add option to the metric adjuster to fallback to collector start time
6 participants
@ridwanmsharif @ArthurSens @Aneurysm9 @dashpole @dehaansa @bogdandrutu