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

Annotate throttled profiles #3956

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Annotate throttled profiles #3956

wants to merge 9 commits into from

Conversation

aleks-p
Copy link
Contributor

@aleks-p aleks-p commented Feb 27, 2025

With #3879 and #3914, distributors reject profiles when an externally controlled ingest limit is reached.

This change annotates the few profiles that are let through in this state, so that users of the profiling data can be made aware that data is being rejected. The information is stored in an extra column in the parquet table, and is applied to v1 and v2 read and write paths.

A few things are still not finished, namely:

  • deduplication in the read path
  • increase test coverage
  • solidify naming and API contract

@@ -311,6 +325,8 @@ type InMemoryProfile struct {
DefaultSampleType int64

Samples Samples

Annotations []string
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory we could achieve the same thing with just using the Comments field above in L322.

I guess comment strings would be in the string table, unsure if that is a good or a bad thing.

I think if we would go for a separate type/field, I think we should make it Key,Value, because that feels more extensible to me than a []string,

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we go with this, maybe at some point we can stop exploiting build Ids for github integration

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.

3 participants