Skip to content

Conversation

@owent
Copy link
Member

@owent owent commented Feb 24, 2023

Fixes #1690
Fixes #1882
Fixes #2008

Should we alse add attribute for TracerProvider::GetTracer and MeterProvider::GetMeter ?

Changes

  • Add attributes for InstrumentationScope
  • Move MakeAttributes from logger.h to key_value_iterable_view.h
  • Add attributes support for InstrumentationScope in GetLogger()
  • Populate Instrumentation Scope attributes

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

+ Move `MakeAttributes` from `logger.h` to `key_value_iterable_view.h`
+ Add attributes support for InstrumentationScope in `GetLogger()`

Signed-off-by: owent <[email protected]>
@owent owent requested a review from a team February 24, 2023 09:25
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #2004 (938747d) into main (075f45d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2004      +/-   ##
==========================================
+ Coverage   87.29%   87.32%   +0.03%     
==========================================
  Files         166      166              
  Lines        4662     4673      +11     
==========================================
+ Hits         4069     4080      +11     
  Misses        593      593              
Impacted Files Coverage Δ
...ude/opentelemetry/common/key_value_iterable_view.h 88.89% <ø> (ø)
...y/sdk/instrumentationscope/instrumentation_scope.h 100.00% <100.00%> (ø)

@owent owent changed the title [WIP] Add attributes for InstrumentationScope Add attributes for InstrumentationScope Feb 24, 2023
@esigo esigo added the size/XL Denotes a PR that changes 500-999 lines. label Feb 26, 2023
Copy link
Member

@lalitb lalitb 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 changes.

* @param attributes
* @return nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>
*/
inline static nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>
Copy link
Member

Choose a reason for hiding this comment

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

nit - with this cleanup, we can also remove

# include "opentelemetry/nostd/span.h"
# include "opentelemetry/nostd/type_traits.h"

from header includes, need to check if there are others too.

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, and the unused headers are removed now.
And #2008 is also included in this PR.

+ Populate Instrumentation Scope attributes

Signed-off-by: owent <[email protected]>
@lalitb lalitb added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) size/XL Denotes a PR that changes 500-999 lines.

Projects

None yet

3 participants