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

Protect start_time and end_time from being set manually by the user #363

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jan 10, 2020

Fixes #361

@ocelotl ocelotl added the sdk Affects the SDK package. label Jan 10, 2020
@ocelotl ocelotl requested a review from a team January 10, 2020 00:22
@ocelotl ocelotl self-assigned this Jan 10, 2020
@ocelotl ocelotl changed the title Protect start_time and end_time from bein set manually by the user Protect start_time and end_time from being set manually by the user Jan 10, 2020
@codecov-io
Copy link

codecov-io commented Jan 10, 2020

Codecov Report

Merging #363 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
+ Coverage   84.82%   84.87%   +0.04%     
==========================================
  Files          38       38              
  Lines        1839     1845       +6     
  Branches      217      217              
==========================================
+ Hits         1560     1566       +6     
  Misses        214      214              
  Partials       65       65
Impacted Files Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 90.12% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c24b3b5...9d986d4. Read the comment docs.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

LGTM

I remember there were some objections against using properties (instead of getters) in the past though (althought that might have been just in the API).

@c24t
Copy link
Member

c24t commented Jan 13, 2020

I remember there were some objections against using properties (instead of getters) in the past though (althought that might have been just in the API).

My big complaint about properties is that if we use them in the API we can't use regular attributes in implementations. Since these attributes only belong to the SDK Span and the user can't safely set them this seems like a fine use for properties.

@c24t c24t merged commit f0ba817 into open-telemetry:master Jan 13, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Couple span's start_time and end_time with start and end methods
5 participants