[chore] updating scrapers to use default method from scraperhelper #22138
Conversation
MovieStoreGuy
requested review from
djaglowski,
dashpole,
codeboten,
dmitryax and
atoulme
as code owners
github-actions
bot
added
receiver/activedirectoryds
receiver/aerospike
receiver/apache
receiver/azuremonitor
receiver/couchdb
receiver/elasticsearch
receiver/filestats
receiver/flinkmetrics
receiver/googlecloudspanner
receiver/haproxy
receiver/httpcheck
github-actions
bot
added
receiver/snowflake
MovieStoreGuy
changed the title
There was a problem hiding this comment.
I like the change suggested in open-telemetry/opentelemetry-collector#7635 but is it really a direct concern that these collection intervals need to be changed? Is there a way to respect the initial delay setting while also allowing each scraper to define its own default collection interval?
If we really do need to make this change, I think a case should be made for it independently of open-telemetry/opentelemetry-collector#7635, and I think we need to consider it a breaking change to every impacted receiver.
So I am of two minds because it presents an opportunity to standardise the defaults for the collector to be consistent and thus reducing the cognitive burden of configuration on end users. The other side of it being that we should respect the default intervals as defined by component owners, the point in case is the snowflake receiver with a 30m interval.
Taking this into consideration, I think it make sense to create the default configuration from the scraper helper and apply the current default timeouts to keep the expected experience consistent. We can follow up with an issue to standardise these and get more feedback from the config sig and code owners. |
MovieStoreGuy
added
the
ready to merge
| # you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: breaking |
There was a problem hiding this comment.
Is this still a breaking change?
If I understand correctly, the change in components' behavior is that they start scraping after 1 second and not after their default collection interval, is this right? In that case, I believe we should call it out in the release notes (using the subtext below?).
There was a problem hiding this comment.
👍🏽 , that makes sense but at this point this is a transparent change until the upstream is merged.
However, I think this missed the v0.78.0 release so we have time to fix this up.
I can correct it in the next release with more details.
There was a problem hiding this comment.
If I understand correctly, the change in components' behavior is that they start scraping after 1 second and not after their default collection interval, is this right?
This comment of mine is actually incorrect 🙂 Not until the core PR gets merged:
Looking at this PR again in detail, it wasn't (meant as) a breaking change, rather a refactoring. The only (accidental, I suppose) breaking change is that the Apache receiver's default collection interval has changed from 10s to 60s. This should probably be fixed in a future PR.
|
Shouldn't we mention the new config property |
| @@ -32,9 +32,7 @@ func NewFactory() receiver.Factory { | |||
|
|
|||
| func createDefaultConfig() component.Config { | |||
| return &Config{ | |||
| ScraperControllerSettings: scraperhelper.ScraperControllerSettings{ | |||
| CollectionInterval: 10 * time.Second, | |||
There was a problem hiding this comment.
Looks like this was missed and effectively the Apache receiver's default collection interval has changed from 10s to 60s?
There was a problem hiding this comment.
Shit, I can revert in a follow up PR.
| ScraperControllerSettings: scraperhelper.ScraperControllerSettings{ | ||
| CollectionInterval: defaultCollectionInterval, | ||
| }, | ||
| ScraperControllerSettings: scraperhelper.NewDefaultScraperControllerSettings(metadata.Type), |
There was a problem hiding this comment.
The defaultCollectionInterval const defined above is now left unused. This could trick someone trying to change it and seeing no effect. We should remove that const.
Let me make an entry in the new release |
My bad, this does not belong in this PR. The |
Description:
Updating components to use the scraper helper config default method.
Link to tracking Issue:
Related to: open-telemetry/opentelemetry-collector#7635
Testing:
Tests have been updated where applicable.
Documentation:
This should be transparent to users so I've avoided it for the time being.