feat(opentelemetry-instrumentation-botocore): Instrument kinesis records#4364
feat(opentelemetry-instrumentation-botocore): Instrument kinesis records#4364mosheshaham-dash0 wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
mosheshaham-dash0
changed the title
| cls._inject_span_into_entry(call_context.params) | ||
|
|
||
| @classmethod | ||
| def _inject_span_into_entry(cls, entry: MutableMapping[str, Any]): |
There was a problem hiding this comment.
This functionality should at the very least be opt-in. There is no guarantee that the user will be serializing data a JSON. Furthermore, from a performance/efficiency point of view, it is very wasteful to serialize JSON just to immediately deserialize it.
There was a problem hiding this comment.
agree. introduced the ENABLE_KINESIS_INSTRUMENTATION env var. by default disabled.
| ################################################################################ | ||
|
|
||
|
|
||
| class _KinesisOperation(abc.ABC): |
There was a problem hiding this comment.
I don't really see the value added here for introducing this ABC and the inheritance hierarchy, especially given that we are only instrumenting two operations here. In my opinion, this makes the implementation details much harder to reason about. Can we refactor this to just use a simple set of helper functions?
There was a problem hiding this comment.
actually, this pattern was copied from other instrumentations in the package, like sns, lambda, dynamodb. I don't think it's too complicated and i think it's better than instrumentations that don't use this pattern like bedrock.
| ################################################################################ | ||
|
|
||
| _OPERATION_MAPPING: Dict[str, _KinesisOperation] = { | ||
| op.operation_name(): op |
There was a problem hiding this comment.
This seems overkill, I would just explicitly list out the operations here.
There was a problem hiding this comment.
you are right, i fixed it. (btw, this was copied from the sns.py insturmentation)
herin049
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
3 participants
Description
This PR adds tracecontext to kinesis PutRecord requests by injecting them into the message payload.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
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 manual testing and added unit testing
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.