[Metrics SDK] Fix hash calculation for nostd::string by lalitb · Pull Request #2999 · open-telemetry/opentelemetry-cpp

lalitb · 2024-07-10T01:40:45Z

Changes

Step 1:

Added test to validate the hash creation issue, where the Hash is being calculated on the address of key , and not it's value as below:

TEST(AttributesHashMap, HashWithKeyValueIterable)

 // Create mock KeyValueIterable instances with the same content but different variables
  MockKeyValueIterable attributes1({{make_unique_string("k1"), make_unique_string("v1")},
                                    {make_unique_string("k2"), make_unique_string("v2")}});
  MockKeyValueIterable attributes2({{make_unique_string("k1"), make_unique_string("v1")},
                                    {make_unique_string("k2"), make_unique_string("v2")}});
  MockKeyValueIterable attributes3({{make_unique_string("k1"), make_unique_string("v1")},
                                    {make_unique_string("k2"), make_unique_string("v2")},
                                    {make_unique_string("k3"), make_unique_string("v3")}});

Most existing tests directly use key and value as literals, and these literals share a common address from the read-only data segment. This masked the issue, and it is also the most common use case where metric attributes are statically created and used during measurement recording.

CI failure - https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/9870060037/job/27255004557?pr=2999

[ RUN      ] AttributesHashMap.HashWithKeyValueIterable
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/metrics/attributes_hashmap_test.cc:142: Failure
Expected equality of these values:
  hash1
    Which is: 7942177087367374725
  hash2
    Which is: 7942177087698536463

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/metrics/attributes_hashmap_test.cc:144: Failure
Expected equality of these values:
  hash1
    Which is: 7942177087367374725
  hash3
    Which is: 7942177087497598195

Step 2:
The fix is to replace the hash calculation:

GetHash(seed, key.data());

with

GetHash(seed, key);

and so directly use the custom hash implementation for nostd::string_view. Though it comes with a cost, as this custom hash-implementation creates an intermediate string. This needs to be revisited.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2024-07-10T01:53:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.66%. Comparing base (497eaf4) to head (bb92dc5).
Report is 101 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2999      +/-   ##
==========================================
+ Coverage   87.12%   87.66%   +0.55%     
==========================================
  Files         200      190      -10     
  Lines        6109     5858     -251     
==========================================
- Hits         5322     5135     -187     
+ Misses        787      723      -64

Files	Coverage Δ
...clude/opentelemetry/sdk/common/attributemap_hash.h	`90.00% <100.00%> (+6.13%)`	⬆️

... and 120 files with indirect coverage changes

…pp into fix-overflow-hash

marcalff · 2024-07-10T09:58:18Z

Is there a way to make sure that GetHash(seed, key); is the same, with:

const char *key="foo";
std::string_view key{"foo"};
nostd::string_view key{"foo"};
std::string key{"foo"};

Maybe instantiate the GenHash template explicitly for all string types that matter.

ThomsonTan · 2024-07-10T13:53:44Z

sdk/include/opentelemetry/sdk/common/attributemap_hash.h

        {
          return true;
        }
-        GetHash(seed, key.data());


In the GetHash function, could we assert that arg should not be a pointer?

We can add it, but since this is an internal API, it might not be necessary as long as we ensure it's used correctly.

Thinking again, we do need to support the GetHash function with char pointer, as the AttributeValue enum contains it.

If we do need supporter pointer, perhaps create a dedicated specialization for it? Then a difference hash function could be applied to pointers.

yes, it is done in the latest commit.

lalitb · 2024-07-10T15:41:05Z

Is there a way to make sure that GetHash(seed, key); is the same, with:

const char *key="foo";

std::string_view key{"foo"};

nostd::string_view key{"foo"};

std::string key{"foo"};

Maybe instantiate the GenHash template explicitly for all string types that matter.

I believe we need template specialization for const char *, as it is the valid AttributeValue type, rest should work with default hashing implementation, I can update the tests to validate it.

lalitb · 2024-07-15T21:08:53Z

bazel-asan is failing. Will check that-

//sdk/test/metrics:attributes_hashmap_test FAILED in 0.5s
/home/runner/.cache/bazel/e6b8d6759295e14b76bc8cb98b604748/execroot/_main/bazel-out/k8-fastbuild-asan/testlogs/sdk/test/metrics/attributes_hashmap_test/test.log

marcalff · 2024-07-17T17:27:03Z

LGTM, thanks for the fix.

lalitb added 2 commits July 9, 2024 18:30

add test to validate cardinaity limit

1bf392a

format

140d8aa

lalitb requested a review from a team July 10, 2024 01:40

lalitb and others added 11 commits July 9, 2024 19:15

warning

0e595a3

Format

e269a9e

maint mode CI

60cac1c

modify test to reproduce the issue

2058aa3

Merge branch 'fix-overflow-hash' of github.com:lalitb/opentelemetry-c…

6f7db17

…pp into fix-overflow-hash

Format

1c61fa5

remove vscode settings

ddc0634

remove redundant test

2e678b4

remove unused code

fd733ea

fix to calculate hash on string_view

b55f513

remove iostream

9e28d46

ThomsonTan reviewed Jul 10, 2024

View reviewed changes

lalitb added 4 commits July 10, 2024 13:59

template specialization for const char *, and varous string type tests

f9d636f

unused variable warning

74839b4

more warnings

f4bb581

format

63819b3

lalitb changed the title ~~Add more tests to validate cardinality limit~~ [Metrics SDK] Fix hash calculation for nostd::string Jul 15, 2024

lalitb added the pr:please-review This PR is ready for review label Jul 15, 2024

Merge branch 'main' into fix-overflow-hash

a872ee8

marcalff mentioned this pull request Jul 15, 2024

[RELEASE] Prepare patch release 1.16.1 #3006

Closed

ThomsonTan approved these changes Jul 15, 2024

View reviewed changes

lalitb added 2 commits July 16, 2024 16:06

fix use-after-stack-scope

fcc9fd1

format

c170c76

lalitb added 2 commits July 16, 2024 17:10

another try

a2590e3

format

bb92dc5

lalitb added ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) and removed pr:please-review This PR is ready for review labels Jul 17, 2024

lalitb merged commit d8ae09e into open-telemetry:main Jul 17, 2024

BrewTestBot mentioned this pull request Jul 17, 2024

opentelemetry-cpp 1.16.1 Homebrew/homebrew-core#177660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics SDK] Fix hash calculation for nostd::string #2999

[Metrics SDK] Fix hash calculation for nostd::string #2999
lalitb merged 22 commits intoopen-telemetry:mainfrom
lalitb:fix-overflow-hash

lalitb commented Jul 10, 2024 •

edited

Loading

Uh oh!

codecov bot commented Jul 10, 2024 •

edited

Loading

Uh oh!

marcalff commented Jul 10, 2024 •

edited

Loading

Uh oh!

ThomsonTan Jul 10, 2024

Uh oh!

lalitb Jul 10, 2024 •

edited

Loading

Uh oh!

lalitb Jul 10, 2024

Uh oh!

ThomsonTan Jul 10, 2024

Uh oh!

lalitb Jul 10, 2024

Uh oh!

lalitb commented Jul 10, 2024 •

edited

Loading

Uh oh!

lalitb commented Jul 15, 2024

Uh oh!

marcalff commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lalitb commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov bot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marcalff commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomsonTan Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

lalitb Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

lalitb Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

lalitb commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb commented Jul 15, 2024

Uh oh!

marcalff commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lalitb commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

marcalff commented Jul 10, 2024 •

edited

Loading

lalitb Jul 10, 2024 •

edited

Loading

lalitb commented Jul 10, 2024 •

edited

Loading