Skip to content

Conversation

nikhilbhatia08
Copy link
Contributor

@nikhilbhatia08 nikhilbhatia08 commented Sep 16, 2025

Fixes #3635

nostd::string_view and access the value with key.data() but this may not be null terminated. So we need a custom hash and equality for the view container

Changes

Please provide a brief description of the changes here.

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

@nikhilbhatia08 nikhilbhatia08 requested a review from a team as a code owner September 16, 2025 03:34
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.10%. Comparing base (74d348a) to head (7320c35).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ntelemetry/sdk/metrics/view/attributes_processor.h 85.72% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3643      +/-   ##
==========================================
+ Coverage   90.08%   90.10%   +0.03%     
==========================================
  Files         220      221       +1     
  Lines        7110     7129      +19     
==========================================
+ Hits         6404     6423      +19     
  Misses        706      706              
Files with missing lines Coverage Δ
...de/opentelemetry/sdk/common/custom_hash_equality.h 100.00% <100.00%> (ø)
...ntelemetry/sdk/metrics/view/attributes_processor.h 84.62% <85.72%> (-1.87%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nikhilbhatia08
Copy link
Contributor Author

std::unordered_map does not support heterogeneous lookup is_transparent before cpp20, even with custom hash/equality older standard libraries (especially libc++ on macOS and pre cpp20) do not accept string_view keys without materializing a std::string. Any ideas on how to make this work with even before versions of cpp20 ?

@lalitb lalitb requested a review from Copilot September 19, 2025 14:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the attributes processor where nostd::string_view keys were not being properly hashed and compared when used in std::unordered_map. The issue occurred because string_view.data() may not be null-terminated, causing incorrect lookups and comparisons.

  • Implements custom StringViewHash and StringViewEqual functors for safe string_view operations
  • Adds cross-platform heterogeneous lookup helper functions for compatibility
  • Updates all unordered_map declarations to use the new custom hash and equality functors

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h Adds custom hash/equality functors and updates FilteringAttributesProcessor classes to use them
sdk/test/metrics/attributes_processor_test.cc Updates test unordered_map declarations to use new custom functors
sdk/test/metrics/sum_aggregation_test.cc Updates test unordered_map declarations to use new custom functors

Comment on lines 44 to 45
return std::hash<opentelemetry::nostd::string_view>{}(
opentelemetry::nostd::string_view{sv.data(), sv.size()});
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

Creating a new string_view from an existing string_view is redundant and may not address the original issue. The problem is that string_view may not be null-terminated, but this code still uses the same data pointer. Consider using std::string construction instead: return std::hash<std::string>{}(std::string{sv.data(), sv.size()});

Suggested change
return std::hash<opentelemetry::nostd::string_view>{}(
opentelemetry::nostd::string_view{sv.data(), sv.size()});
// Materialize to std::string to ensure safe hashing (string_view may not be null-terminated)
return std::hash<std::string>{}(std::string{sv.data(), sv.size()});

Copilot uses AI. Check for mistakes.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Please document where the new hash function comes from.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, see minor changes.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@marcalff marcalff changed the title [fix]: custom hash and equality for attribute processor [SDK] custom hash and equality for attribute processor Sep 23, 2025
@marcalff marcalff merged commit 661a58a into open-telemetry:main Sep 23, 2025
66 checks passed
malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request Sep 24, 2025
[SDK] custom hash and equality for attribute processor (open-telemetry#3643)
@nikhilbhatia08 nikhilbhatia08 deleted the custom_hash_equality branch September 25, 2025 18:16
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.

[Metrics] Attributes Processor needs custom hash and equality

3 participants