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

feat(query): add TenantIngestionMetering query throttle #1310

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

alextheimer
Copy link
Contributor

@alextheimer alextheimer commented Dec 15, 2021

Pull Request checklist

  • The commit(s) message(s) follows the contribution guidelines ?
  • Tests for the changes have been added (for bug fixes / features) ?
  • Docs have been added / updated (for bug fixes / features) ?

Current behavior :
TenantIngestionMetering queries might consistently take longer than the static timeout duration. No timeseries would be published.

New behavior :
Adds a throttling subclass to monitor query timeouts. This class adjusts the query frequency and timeout duration according to the count of timeouts in a lookback window.

@alextheimer alextheimer force-pushed the card-throttle branch 2 times, most recently from aea4659 to ca17e67 Compare December 15, 2021 02:44
@alextheimer alextheimer changed the title ==WIP== // feat(query): add TenantIngestionMetering query throttle feat(query): add TenantIngestionMetering query throttle Dec 15, 2021
@alextheimer alextheimer changed the title feat(query): add TenantIngestionMetering query throttle ==WIP== // feat(query): add TenantIngestionMetering query throttle Dec 15, 2021
@alextheimer alextheimer changed the title ==WIP== // feat(query): add TenantIngestionMetering query throttle feat(query): add TenantIngestionMetering query throttle Dec 15, 2021
@alextheimer alextheimer marked this pull request as ready for review December 16, 2021 01:10
settings,
() => { _datasetInfo.map{ case (dsRef, _) => dsRef}.toIterator },
() => { _coordinators.head._2 })
inst.schedulePeriodicPublishJob()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real reason to require this method to be called after initialization.

Comment on lines 37 to 43
private val SCHED_INIT_DELAY = ASK_TIMEOUT // time until first job is scheduled
private val SCHED_DELAY = ASK_TIMEOUT // time between all jobs after the first

private val CLUSTER_TYPE = settings.config.getString("cluster-type")

private val METRIC_ACTIVE = "active_timeseries_by_tenant"
private val METRIC_TOTAL = "total_timeseries_by_tenant"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to companion object.

@alextheimer alextheimer changed the title feat(query): add TenantIngestionMetering query throttle ==WIP== // feat(query): add TenantIngestionMetering query throttle Jan 4, 2022
@alextheimer alextheimer marked this pull request as draft January 4, 2022 18:38
@alextheimer alextheimer changed the title ==WIP== // feat(query): add TenantIngestionMetering query throttle feat(query): add TenantIngestionMetering query throttle Jan 4, 2022
@alextheimer alextheimer marked this pull request as ready for review January 4, 2022 22:02
object TenantIngestionMetering {
protected val METRIC_ACTIVE = "active_timeseries_by_tenant"
protected val METRIC_TOTAL = "total_timeseries_by_tenant"
protected val PARALLELISM = 8 // number of datasets queried in parallel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure what this should be-- should I make it configurable? Or get the number of cores from the JRE?

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

Successfully merging this pull request may close these issues.

1 participant