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.

Sign up for GitHub

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

Jump to bottom

ReadableSpan events and links now return a tuple #2215

Merged
merged 6 commits into from Oct 22, 2021
Merged

ReadableSpan events and links now return a tuple #2215

merged 6 commits into from Oct 22, 2021

Conversation

Arnatious
Copy link
Contributor

Description

ReadableSpan events and links attributes now return a sequence, as described, removing a TypeError.

No longer attempt to wrap Links and Events in MappingProxy's since they are not descendants of Mapping like Attributes.

Fixes #2214

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ran tox -e py38-opentelemetry-sdk -- trace/test_trace.py::TestReadableSpan (tested on Python 3.8.10 on Ubuntu on WSL2)

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8

  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md

  • Yes. - Link to PR:

  • No.

Checklist:

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

@Arnatious Arnatious requested a review from a team as a code owner October 16, 2021 09:35
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 16, 2021 •
edited

CLA Signed

The committers are authorized under a signed CLA.

@Arnatious Arnatious force-pushed the fix-readablespan-events-links branch from dd49d29 to 1de5c5b Compare October 16, 2021 09:38
@ocelotl ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Oct 18, 2021
@ocelotl
Copy link
Contributor

ocelotl commented Oct 18, 2021

I think this PR needs a changelog entry ✌️

@lzchen lzchen enabled auto-merge (squash) October 18, 2021 16:26
auto-merge was automatically disabled October 18, 2021 21:15

Head branch was pushed to by a user without write access

@Arnatious
ReadableSpan events and links now return a tuple
48faadb 
Removed MappingProxy since events and links are not mappings

Signed-off-by: Ted Kern <tkern@arnatious.com>
@Arnatious Arnatious force-pushed the fix-readablespan-events-links branch from 1be9a6b to 48faadb Compare October 18, 2021 21:49
@Arnatious
Copy link
Contributor Author

Added changelog entry!

@owais owais enabled auto-merge (squash) October 19, 2021 00:42
@ocelotl
Copy link
Contributor

ocelotl commented Oct 19, 2021

Please fix lint and we can get this merged in, @Arnatious ✌️

@Arnatious
fix lint
bd4fdfd 
Signed-off-by: Ted Kern <tkern@arnatious.com>
auto-merge was automatically disabled October 20, 2021 00:21

Head branch was pushed to by a user without write access

@Arnatious
Copy link
Contributor Author

Arnatious commented Oct 20, 2021 •
edited

Re:fixed lint, looks like I missed whitespace first time.

Separate issue I'll raise: tox -e lint fails because of

ERROR: Cannot install httpretty because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested httpretty
    The user requested (constraint) httpretty~=1.0

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies

@ocelotl
Copy link
Contributor

ocelotl commented Oct 21, 2021 •
edited

Here is how to fix black and isort lint problems:

  1. cd to the root of the opentelemetry-python repo
  2. tox -e lint
  3. source .tox/lint/bin/activate
  4. black .
  5. isort .
  6. deactivate

Commit the generated changes.

@owais owais merged commit a002f7a into open-telemetry:main Oct 22, 2021
@aabmass aabmass mentioned this pull request Oct 29, 2021
Merged
aabmass added a commit that referenced this pull request Oct 29, 2021
@aabmass @lzchen
@chalin @robertsben @owais @Arnatious @ocelotl @JamesJHPark @NathanielRN
Merge main into metrics_new (#2250)
aff87a7 
* updating changelogs and version to 1.6.1-0.25b1 (#2223)

* default value is invalid, was renamed (#2226)

* Website: support GH page links to canonical src, and aliases (#2224)

* Fix opentracing shim references (#2180)

* Prepare release v1.6.2 and 0.25b2 (#2230)

* update changelog to note the rename of entrypoints in 1.6.0 (#2228)

* add note about proto version to README.rst (#2232)

* ReadableSpan events and links now return a tuple (#2215)

* ReadableSpan events and links now return a tuple

Removed MappingProxy since events and links are not mappings

Signed-off-by: Ted Kern <tkern@arnatious.com>

* fix lint

Signed-off-by: Ted Kern <tkern@arnatious.com>

* fix lint

* fix lint

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Co-authored-by: Leighton Chen <lechen@microsoft.com>

* adding CI testing for python 3.10 (#2207)

* remove `X-B3-ParentSpanId` for B3 propagator as per OpenTelemetry Specification (#2237)

* remove X-B3-ParentSpanId for B3 propagator as per OpenTelemetry specification

* revert changes - run tests

* re-commit changes

* add entry to CHANGELOG

* remove ParentSpanId in B3SingleFormat

* Remove lingering distro pkg files (#2241)

Co-authored-by: Leighton Chen <lechen@microsoft.com>
Co-authored-by: alrex <aboten@lightstep.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Ben Roberts <robertsben@users.noreply.github.com>
Co-authored-by: Owais Lone <owais@users.noreply.github.com>
Co-authored-by: Ted Kern <tkern@arnatious.com>
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Co-authored-by: James <45812677+JamesJHPark@users.noreply.github.com>
Co-authored-by: (Eliseo) Nathaniel Ruiz Nowell <nathanielruiz98@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owais owais owais approved these changes

@lzchen lzchen lzchen approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

Assignees
No one assigned
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ReadableSpan 'links' and 'events' properties cause TypeError
5 participants
@Arnatious @ocelotl @owais @lzchen @srikanthccv