[xuan-cao-swi]

Description

Resolves #2359

1. add attributes_key option for views
2. attributes_key can have three format:

Array: attribute_keys = ['env', 'host'] # this will be treated as allow list -> ['env', 'host']
Hash:  attributes_key = {'a' => true, 'b' => 'c' } # this will be treated as allow list -> ['a', 'b']
Hash:  attributes_key = { included: ['env'], excluded: ['b'] } # this will be treated as two list: allowlist and denylist 

view will filter out the attributes that is for reporting if the attributes key is in denylist or not in allowlist (e.g. view_attributes = view.filter_attributes(attributes))

[mwear]

I left a few comments in-line. Since this changes what attribute_keys does for anyone passing a Hash today (values were merged in, now the keys filter the stream), can you mark the title as breaking, e.g. fix(metrics_sdk)!:, so it's called out in the changelog?

[mwear]

overlap isn't defined, so a NameError will be raised if this code path executes.

The README and the add_view docs also say this raises an ArgumentError, but the intent here looks like warn and treat the key as excluded. The spec allows either, so I have a slight preference for raising an ArgumentError, since that's what's already documented. Either way, can you make the code and docs agree and add a test for the overlap case?

[xuan-cao-swi]

Update to use overlap and added the test case for overlap.
I still log the warning because the only concern is that don't want to break user's application, although I am open to raise the error anyway since the next comment/change also raise the error

[mwear]

If a Hash mixes the two forms, e.g. { included: ['a'], 'b' => nil }, this falls through to the legacy allow-list path and the allow-list becomes ['included', 'b'], so a gets dropped. Similarly, { 'included' => nil }, meant as a legacy allow-list for an attribute named included, becomes no filter at all because of the symbol conversion above. I think raising an ArgumentError when included/excluded are mixed with other keys would be clearer than guessing. What do you think?

[xuan-cao-swi]

Yes, I think simplification make sense. Raising error here but I am open to just log the warning to avoid break user's application (e.g. log the error and don't ignore anything)

[xuan-cao-swi]

p.s. either log or raise the error for both case