Skip to content

Remove default parameters from Recordable interface#208

Merged
reyang merged 11 commits into
open-telemetry:masterfrom
nadiaciobanu:interface-update
Jul 28, 2020
Merged

Remove default parameters from Recordable interface#208
reyang merged 11 commits into
open-telemetry:masterfrom
nadiaciobanu:interface-update

Conversation

@nadiaciobanu
Copy link
Copy Markdown
Contributor

This previous PR added support for links and event attributes in the base Recordable. Some parameters of the virtual functions were assigned defaults (for example the default timestamp is now and default attributes are an empty map). This was done to conform to the OpenTelemetry spec (see Add Events and Add Links).

After further research, I found that many people strongly recommend against using default parameters in virtual functions, since default parameters depend on the static type of the object, whereas the virtual function dispatched depends on the dynamic type (see a discussion here). In fact, the Google C++ Style Guide goes as far as to ban default parameters on virtual functions.

This PR replaces the default parameters in the Recordable virtual functions with function overriding. I believe this will lead to more consistent and predictable behaviour. It will also remove the need for each derived class of the base Recordable to re-define the default parameters.

I've also added a static empty map to the base Recordable, to avoid creating a new empty map every time the default attributes are used.

@nadiaciobanu nadiaciobanu requested a review from a team July 23, 2020 18:43
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 23, 2020

Codecov Report

Merging #208 into master will increase coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   92.30%   92.31%   +0.01%     
==========================================
  Files         101      102       +1     
  Lines        3169     3174       +5     
==========================================
+ Hits         2925     2930       +5     
  Misses        244      244              
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/span_data.h 98.33% <50.00%> (ø)
...nclude/opentelemetry/sdk/common/empty_attributes.h 100.00% <100.00%> (ø)
sdk/include/opentelemetry/sdk/trace/recordable.h 100.00% <100.00%> (ø)
sdk/test/trace/span_data_test.cc 100.00% <100.00%> (ø)

Comment thread sdk/include/opentelemetry/sdk/trace/recordable.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/recordable.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/empty_attributes.h Outdated
Comment thread sdk/test/common/empty_attributes_test.cc Outdated
Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 24, 2020
@nadiaciobanu
Copy link
Copy Markdown
Contributor Author

@pyohannes @IlyaKobelevskiy please let me know if you have any further thoughts!

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

This looks good. The current flaky PR builder shouldn't block a merge. @reyang

@reyang reyang merged commit b096d8e into open-telemetry:master Jul 28, 2020
ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020
ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020
@nadiaciobanu nadiaciobanu deleted the interface-update branch August 4, 2020 16:32
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
…onorepo

Update dependency protobuf to v30.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants