Skip to content

feat(wsgi): low cardinality span name and name override#837

Merged
lzchen merged 14 commits intoopen-telemetry:masterfrom
DataDog:majorgreys/pr440
Jun 23, 2020
Merged

feat(wsgi): low cardinality span name and name override#837
lzchen merged 14 commits intoopen-telemetry:masterfrom
DataDog:majorgreys/pr440

Conversation

@majorgreys
Copy link
Copy Markdown
Contributor

Closes #440

@majorgreys majorgreys requested a review from a team June 18, 2020 01:12
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jun 18, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Copy Markdown
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

A couple minor changes requested. Only one comment requires attention, there may be a bug on the implementation of get_default_span_name, so please just respond to it, @majorgreys.

Comment thread ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py
Comment thread ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py Outdated
Comment on lines +154 to +156
def get_predefined_span_name(scope):
# pylint: disable=unused-argument
return span_name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this could be replaced with a Mock:

In [4]: from unittest.mock import Mock

In [5]: the_mock = Mock(**{"return_value": "Dymaxion"})

In [6]: the_mock({"a": "b"})
Out[6]: 'Dymaxion'

Copy link
Copy Markdown
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Nice

@majorgreys majorgreys requested a review from ocelotl June 22, 2020 14:14
Copy link
Copy Markdown
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Please review the comments, @majorgreys.

Comment thread ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py Outdated
Comment thread ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py Outdated
@lzchen lzchen merged commit 3c9f99d into open-telemetry:master Jun 23, 2020
cnnradams pushed a commit to cnnradams/opentelemetry-python that referenced this pull request Jul 2, 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants