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

Upstream internal jsonnet changes #6764

Merged
merged 10 commits into from
Nov 29, 2023
Merged

Upstream internal jsonnet changes #6764

merged 10 commits into from
Nov 29, 2023

Conversation

pstibrany
Copy link
Member

What this PR does

This PR upstreams some of the changes that we use internally.

  • Increase JAEGER_REPORTER_MAX_QUEUE_SIZE to 5000 to avoid dropping of spans

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@pstibrany pstibrany requested a review from a team as a code owner November 28, 2023 14:52
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM (modulo a nit)

@@ -69,6 +69,7 @@
storage_azure_account_key: error 'must specify Azure account key',

jaeger_agent_host: null,
querier_jaeger_reporter_max_queue_size: 5000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think we need to allow to configure it. An user can always override it in querier_env_map.

@leizor
Copy link
Contributor

leizor commented Nov 28, 2023

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
…-matchers-cache-max-bytes

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
…-matchers-cache-max-bytes

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
@pstibrany pstibrany requested a review from a team as a code owner November 29, 2023 12:02
@pstibrany
Copy link
Member Author

@56quarters @pracucci I've upstreamed another change from our jsonnet:

      'blocks-storage.tsdb.head-postings-for-matchers-cache-max-bytes': 100 * 1024 * 1024,
      'blocks-storage.tsdb.block-postings-for-matchers-cache-max-bytes': 100 * 1024 * 1024,

However, instead of changing jsonnet / helm, I've opted to change default value used by ingester. WDYT?

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

The default value config change LGTM (modulo a couple of nits)

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 90 to 96
// This increase is most useful on Mimir clusters with 1 tenant. If the tenant runs very high cardinality
// queries (e.g. a query touching 1M series / ingester) then with the default cache
// size of 10MB we may not be able to effectively use the cache.
//
// A single cached posting takes about 9 bytes in the cache, on average. The default max cache size as number of items
// is 100, so having a 100MB cache per-tenant for the TSDB Head means we can cache 100MB / 100 / 9 = 116k postings
// per cached entry on average.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rephrase it. It says "with the default cache size of 10MB" which is no more the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, thanks. Updated the comment.

pstibrany and others added 2 commits November 29, 2023 14:54
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
@pstibrany pstibrany merged commit a2ba613 into main Nov 29, 2023
28 checks passed
@pstibrany pstibrany deleted the upstream-jsonnet-changes branch November 29, 2023 15:04
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.

4 participants