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.

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

[chore] Updates to scrape duration #23030

Conversation

MovieStoreGuy
Copy link
Contributor

Description:

  • This is to prepare for the future release of core that includes scraping from collector start.
  • Fixes accidental change to apache receiver's default collection interval.

Link to tracking Issue:

open-telemetry/opentelemetry-collector#7635

Testing:
NA

Documentation:
NA

@MovieStoreGuy MovieStoreGuy requested a review from a team June 2, 2023 07:28
@MovieStoreGuy MovieStoreGuy requested a review from djaglowski as a code owner June 2, 2023 07:28
@MovieStoreGuy MovieStoreGuy force-pushed the msg/add-scrape-interval-changelog branch from adb630b to 902350e Compare June 2, 2023 07:29
# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps useful to add that the change was mistakenly added in v0.78.0?

@MovieStoreGuy
Copy link
Contributor Author

Don't merge til open-telemetry/opentelemetry-collector#7635 is merged.

@andrzej-stencel
Copy link
Member

I think this PR also needs to update the docs of all the affected receivers to add the new initial_delay configuration option.

@andrzej-stencel
Copy link
Member

This is great. Now I wonder: is this a breaking change? 🤔 It changes the time of the first scrape for all the affected components - from the value of collection_interval (which is 10s or 60s for most components) to 1s. Is this change OK? Or should we rather make the default initial_delay be the same as collection_interval so that there's no change in (the default) behavior? 🤔 Perhaps it's fine, I cannot think of an issue that might arise from this change.

@songy23 songy23 added the ready to merge Code review completed; ready to merge by maintainers label Jun 2, 2023
@dmitryax dmitryax merged commit eacf9c3 into open-telemetry:main Jun 2, 2023
@github-actions github-actions bot added this to the next release milestone Jun 2, 2023
@MovieStoreGuy
Copy link
Contributor Author

This is great. Now I wonder: is this a breaking change? 🤔 It changes the time of the first scrape for all the affected components - from the value of collection_interval (which is 10s or 60s for most components) to 1s. Is this change OK? Or should we rather make the default initial_delay be the same as collection_interval so that there's no change in (the default) behavior? 🤔 Perhaps it's fine, I cannot think of an issue that might arise from this change.

The interval changes would only really impact users who have larger intervals since their current expectations is to wait that to capture data. The gut feeling is that collection intervals under 1m don't notice since something like 10s over a duration of an hour is hidden is most observability tools rollups or aggregations.

Caleb-Hurshman pushed a commit to observIQ/opentelemetry-collector-contrib that referenced this pull request Jul 6, 2023
- This is to prepare for the future release of core that includes scraping from collector start.
- Fixes accidental change to apache receiver's default collection interval.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment