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

Fix decumulating bucket on multiple contexts #4446

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

mfpierre
Copy link
Contributor

What does this PR do?

Fix overidding buckets when they have different tag context

Motivation

Support histograms having a cardinality > 1

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

Copy link
Contributor

@clamoriniere clamoriniere left a comment

Choose a reason for hiding this comment

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

just a small comment on the unit-test part

@mfpierre mfpierre force-pushed the mfpierre/fix-context-decumulate-buckets branch from b336481 to d98448d Compare August 28, 2019 13:01
clamoriniere
clamoriniere previously approved these changes Aug 28, 2019
@albertvaka
Copy link
Contributor

If you want to include this in rc3, you can still delete & recreate the tag to include it (rc3 hasn't been built yet).

# hence we remove the "le" tag
hash_dict = tags.copy()
del hash_dict["le"]
return hash(frozenset(sorted(iteritems(hash_dict))))
Copy link
Contributor

@ofek ofek Aug 28, 2019

Choose a reason for hiding this comment

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

    def _compute_bucket_hash(self, tags):
        # we need the unique context for all the buckets
        # hence we ignore the `le` tag
        return hash(frozenset(sorted((k, v) for k, v in iteritems(tags) if k != 'le')))

@mfpierre mfpierre merged commit e423522 into master Aug 28, 2019
@mfpierre mfpierre deleted the mfpierre/fix-context-decumulate-buckets branch August 28, 2019 13:24
ChristineTChen pushed a commit that referenced this pull request Aug 28, 2019
* Fix decumulating bucket on multiple contexts

* Add test for hash tag order
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