Skip to content

[tracing] Add span event APIs and wrapper#8470

Merged
diemol merged 10 commits into
SeleniumHQ:trunkfrom
pujagani:tracing_add_spanevents
Jul 10, 2020
Merged

[tracing] Add span event APIs and wrapper#8470
diemol merged 10 commits into
SeleniumHQ:trunkfrom
pujagani:tracing_add_spanevents

Conversation

@pujagani

@pujagani pujagani commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Description

Updated existing Span interface and OpenTelemetrySpan to add APIs for creating events in the span.
The APIs allow to add attributes to the event in the span. The attributes are stored using a Map. To enable use of single Map while creating event attributes for different data types of attributes (String , long, double, boolean), EventAttribute interface and data types specific implementations are added.

Motivation and Context

The current tracing wrapper around OpenTelemetry does not provide APIs to add events to the spans. This PR provides the APIs to instrument events in the spans with and without attributes. Events are crucial to drill down and triage an issue. Events help in documenting a moment at that timestamp within a span.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@CLAassistant

CLAassistant commented Jun 30, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@shs96c shs96c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great. Thank you. I don't think we need the variant with the timestamp since we never need to futz with that in our own code, so I'd suggest removing it, but this looks good.

Comment thread java/client/src/org/openqa/selenium/remote/tracing/Span.java Outdated
Comment thread java/client/src/org/openqa/selenium/remote/tracing/empty/NullSpan.java Outdated
Comment thread java/client/test/org/openqa/selenium/remote/tracing/opentelemetry/TracerTest.java Outdated
@pujagani pujagani force-pushed the tracing_add_spanevents branch from 99665e3 to 0db9d7f Compare July 1, 2020 10:07
@pujagani pujagani changed the base branch from master to trunk July 6, 2020 05:22

@diemol diemol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added a couple of comments.

Comment thread java/client/src/org/openqa/selenium/remote/tracing/BUILD.bazel Outdated
Comment thread java/client/src/org/openqa/selenium/remote/tracing/Span.java Outdated
@SeleniumHQ SeleniumHQ deleted a comment from subratkrgouda Jul 10, 2020
@diemol diemol merged commit 4c0630f into SeleniumHQ:trunk Jul 10, 2020
titusfortner pushed a commit to titusfortner/selenium that referenced this pull request Aug 13, 2020
* [tracing] Add span event APIs and wrapper

* [tracing] Add span event APIs with attributes

* [tracing] Add EventAttribute Creator and EventAttributeValue class.

* [Grid] Removing unneeded dependency [skip ci]

* [Grid] Moving type enum because it only applies to EventAttributeValue [skip ci]

* [Grid] Simplifying class and method names (the parameter says the type) [skip ci]

* [Grid] An attempt to use strong typing, avoid casting, since we only have 4 type cases. We can iterate on it. [skip ci]

Co-authored-by: David Burns <david.burns@theautomatedtester.co.uk>
Co-authored-by: Diego Molina <diemol@gmail.com>
Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
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.

5 participants