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

feat(views): add FilteringAttributesProcessor #2733

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

This PR is part of the work on #2592 and adds a FilteringAttributesProcessor, to fulfill the following spec requirement for view configuration:

A list of attribute keys (optional). If provided, the attributes that are not in the list will be ignored. If not provided, all the attribute keys will be used by default (TODO: once the Hint API is available, the default behavior should respect the Hint if it is available).

Short description of the changes

Adds a FilteringAttributesProcessor that can be passed to a View to filter attributes.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@pichlermarc pichlermarc requested a review from a team January 24, 2022 16:20
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #2733 (c49e0c2) into main (8daaddc) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c49e0c2 differs from pull request most recent head 611da39. Consider uploading reports for the commit 611da39 to get more accurate results

@@            Coverage Diff             @@
##             main    #2733      +/-   ##
==========================================
- Coverage   93.28%   93.27%   -0.01%     
==========================================
  Files         158      158              
  Lines        5434     5443       +9     
  Branches     1141     1141              
==========================================
+ Hits         5069     5077       +8     
- Misses        365      366       +1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...y-sdk-metrics-base/src/view/AttributesProcessor.ts 100.00% <0.00%> (ø)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM with a minor suggestion

process(incoming: Attributes, _context:Context): Attributes {
const filteredAttributes: Attributes = {};
for(const allowedAttributeName of this._allowedAttributeNames){
if(allowedAttributeName in incoming){
Copy link
Member

Choose a reason for hiding this comment

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

I would probably prefer to use something like Object.hasOwnProperty in order to avoid any weirdness with the object prototype. You would want to implement it respecting https://eslint.org/docs/rules/no-prototype-builtins like this maybe:

Suggested change
if(allowedAttributeName in incoming){
if(Object.prototype.hasOwnProperty.call(incoming, allowedAttributeName)){

Copy link
Member

@legendecas legendecas Jan 25, 2022

Choose a reason for hiding this comment

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

I submitted a PR to explicitly document that only own-enumerable attribute keys count.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this in the most recent commit.
Should I also make sure that the property is enumerable, or is that something we can assume at the point where attributes are processed? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Probably we can use Object.keys to fetch the own-enumerable keys then filter them with the allowed names list and pick all of them at once. In this way, we also avoid the for-of loop since it is kinda slower than looping over an array since it depends on iterators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I changed it to use Object.keys instead. 🙂

@dyladan dyladan added the enhancement New feature or request label Jan 26, 2022
@dyladan dyladan merged commit e1c32b2 into open-telemetry:main Jan 26, 2022
@dyladan dyladan deleted the filtering-attributes-processor branch January 26, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants