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

Log warning if histogram received cdata #482

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Conversation

yngvar-antonsson
Copy link
Collaborator

@yngvar-antonsson yngvar-antonsson commented Apr 4, 2024

Using cdata values as historgam observations is dangerous, as described in #480. On the other hand, plenty of clients (e.g. tdg) use cdata with this collector. We decided not to throw an error here, at least for now.

I didn't forget about

  • Changelog

Close #480

Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

Thanks. It's still dangerous but it's better than nothing.

@@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- `tnt_election_leader_idle` metric.

- Histogram now logs a warning if `observe` is called with `cdata` value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it "deprecated"

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

It would be nice to have tests, but implementation seems fine

@yngvar-antonsson yngvar-antonsson merged commit e7463e7 into master Apr 8, 2024
64 checks passed
@yngvar-antonsson yngvar-antonsson deleted the fix-cdata-values branch April 8, 2024 09:01
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this pull request Jul 10, 2024
Bump metrics package submodule. Commits from PRs [1-4] affect
Tarantool, the other ones are related to module infrastructure.

1. tarantool/metrics#482
2. tarantool/metrics#483
3. tarantool/metrics#484
4. tarantool/metrics#491

NO_DOC=doc is a part of submodule
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this pull request Jul 10, 2024
Bump metrics package submodule. Commits from PRs [1-4] affect
Tarantool, the other ones are related to module infrastructure.

1. tarantool/metrics#482
2. tarantool/metrics#483
3. tarantool/metrics#484
4. tarantool/metrics#491

NO_DOC=doc is a part of submodule
DifferentialOrange added a commit to DifferentialOrange/tarantool that referenced this pull request Jul 16, 2024
Bump metrics package submodule. Commits from PRs [1-4] affect
Tarantool, the other ones are related to module infrastructure.

1. tarantool/metrics#482
2. tarantool/metrics#483
3. tarantool/metrics#484
4. tarantool/metrics#491

NO_DOC=doc is a part of submodule
Totktonada pushed a commit to tarantool/tarantool that referenced this pull request Jul 22, 2024
Bump metrics package submodule. Commits from PRs [1-4] affect
Tarantool, the other ones are related to module infrastructure.

1. tarantool/metrics#482
2. tarantool/metrics#483
3. tarantool/metrics#484
4. tarantool/metrics#491

NO_DOC=doc is a part of submodule
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.

Incorrect handling of cdata values
3 participants