Skip to content

Update pymemcache instrumentation to follow semantic conventions#183

Merged
lzchen merged 3 commits intoopen-telemetry:masterfrom
srikanthccv:memcached-semantic-conv
Nov 17, 2020
Merged

Update pymemcache instrumentation to follow semantic conventions#183
lzchen merged 3 commits intoopen-telemetry:masterfrom
srikanthccv:memcached-semantic-conv

Conversation

@srikanthccv
Copy link
Member

Description

Part of #159

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • tox -e test-instrumentation-pymemcache

@srikanthccv srikanthccv requested review from a team, aabmass and codeboten and removed request for a team November 15, 2020 12:53
@srikanthccv
Copy link
Member Author

cc @lzchen

with tracer.start_as_current_span(
_CMD, kind=SpanKind.INTERNAL, attributes={}
cmd, kind=SpanKind.CLIENT, attributes={}
) as span:
Copy link
Contributor

Choose a reason for hiding this comment

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

From before, does pymemcache not have the concept of name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if I understood you correctly, but memcache supports this list of commands. We are using the command as a span name.

Copy link
Contributor

@lzchen lzchen Nov 16, 2020

Choose a reason for hiding this comment

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

Sorry I was referring to adding the db.name attribute. We should have it as a span attribute and also use it as a span name fallback if the command isn't present for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't have any concept of databases that can be used for db.name.

@lzchen lzchen merged commit c841110 into open-telemetry:master Nov 17, 2020
@srikanthccv srikanthccv deleted the memcached-semantic-conv branch December 5, 2020 18:20
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.

2 participants