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

OTLP: Increase valid created timestamp threshold to 5 minutes #706

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

jesusvazquez
Copy link
Member

Increasing valid threshold for inserting zero's at StartTimeUnixNano on OTLP Write requests.

The reason why we want to do this is that Grafana's $__rate_interval is typically 4 times the scrape interval so assuming an average DPM of 1 we could have the zero sample 4 minutes before the first sample.

This is a small change that might help with usecases where samples are slightly delayed.

Also we've made this threshold configurable now. It defaults to 5 minutes but it can be overiden for special cases.

Increasing valid threshold for inserting zero's at StartTimeUnixNano on OTLP
Write requests.

The reason why we want to do this is that Grafana's $__rate_interval is
typically 4 times the scrape interval so assuming an average DPM of 1 we could
have the zero sample 4 minutes before the first sample.

This is a small change that might help with usecases where samples are slightly
delayed.

Also we've made this threshold configurable now. It defaults to 5 minutes but
it can be overiden for special cases.

Signed-off-by: Jesus Vazquez <[email protected]>
@jesusvazquez jesusvazquez force-pushed the jvp/otlp-ct-zero-timestamp-update branch from 8a97e0b to ce7e117 Compare October 2, 2024 12:54
Signed-off-by: Jesus Vazquez <[email protected]>
@jesusvazquez jesusvazquez marked this pull request as ready for review October 2, 2024 19:50
@krajorama krajorama self-requested a review October 3, 2024 08:14
Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM

web/web.go Outdated
Comment on lines 265 to 266
EnableCreatedTimestampZeroIngestion bool
ValidIntervalCreatedTimestampZeroIngestion time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these two cause conflicts when updating from upstream due to the indenting. Would be nice to eventually clean these up by adding our additions at the end after empty line

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean something like:

	RemoteReadBytesInFrame                     int
	EnableRemoteWriteReceiver                  bool
	EnableOTLPWriteReceiver                    bool

        // Our additions
	EnableCreatedTimestampZeroIngestion        bool
	ValidIntervalCreatedTimestampZeroIngestion time.Duration

Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

thanks

@jesusvazquez jesusvazquez merged commit 35ec40c into main Oct 3, 2024
9 checks passed
@jesusvazquez jesusvazquez deleted the jvp/otlp-ct-zero-timestamp-update branch October 3, 2024 11:40
julienduchesne added a commit that referenced this pull request Jan 8, 2025
…chesne/upstream-pt2

Conflicts:
- api/v1/errors_test.go: #706 with prometheus/prometheus@3b97a63
- web/api/v1/api.go: #684 with prometheus/prometheus@3b97a63. Picked the upstream naming for the parameter (`enableCTZeroIngestion` vs `ctZeroIngestionEnabled`)
julienduchesne added a commit that referenced this pull request Jan 8, 2025
…chesne/upstream-pt2

Conflicts:
- api/v1/errors_test.go: #706 with prometheus/prometheus@3b97a63
- web/api/v1/api.go: #684 with prometheus/prometheus@3b97a63. Picked the upstream naming for the parameter (`enableCTZeroIngestion` vs `ctZeroIngestionEnabled`)
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.

2 participants