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

Implement basic OTLP Recordable #127

Merged
merged 26 commits into from
Jul 2, 2020
Merged

Implement basic OTLP Recordable #127

merged 26 commits into from
Jul 2, 2020

Conversation

nadiaciobanu
Copy link
Contributor

@nadiaciobanu nadiaciobanu commented Jun 24, 2020 •
edited
Loading

Added and implemented SetIds(), SetStartTime() and SetDuration() for the OTLP Recordable. These functions should be enough to create minimal spans that can be sent to the OpenTelemetry Collector. Also added unit tests for the Recordable.

@nadiaciobanu nadiaciobanu requested a review from a team June 24, 2020 19:07
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 24, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@nadiaciobanu nadiaciobanu marked this pull request as draft June 24, 2020 19:08
@codecov
Copy link

codecov bot commented Jun 24, 2020 •
edited
Loading

Codecov Report

Merging #127 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #127   +/-   ##
=======================================
  Coverage   93.50%   93.50%           
=======================================
  Files          71       71           
  Lines        1724     1724           
=======================================
  Hits         1612     1612           
  Misses        112      112

@nadiaciobanu nadiaciobanu marked this pull request as ready for review June 24, 2020 19:12
@nadiaciobanu
Copy link
Contributor Author

I signed my CLA.

g-easy
g-easy reviewed Jun 25, 2020
View reviewed changes
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
IlyaKobelevskiy
IlyaKobelevskiy reviewed Jun 25, 2020
View reviewed changes
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable.cc Outdated Show resolved Hide resolved
IlyaKobelevskiy
IlyaKobelevskiy reviewed Jun 25, 2020
View reviewed changes
exporters/otlp/recordable.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
exporters/otlp/recordable_test.cc Outdated Show resolved Hide resolved
@ZiweiZhao
Copy link

LGTM :)

IlyaKobelevskiy
IlyaKobelevskiy approved these changes Jun 26, 2020
View reviewed changes
Copy link

@IlyaKobelevskiy IlyaKobelevskiy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thanks for addressing comments!

pyohannes
pyohannes reviewed Jun 26, 2020
View reviewed changes
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

This looks good to me, I only have one request about a missing CMake integration.

exporters/otlp/BUILD Show resolved Hide resolved
@nadiaciobanu
Copy link
Contributor Author

nadiaciobanu commented Jun 30, 2020 •
edited
Loading

@pyohannes I've updated my code to address your comment, please let me know if there's anything else!

pyohannes
pyohannes approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@nadiaciobanu
Copy link
Contributor Author

@reyang This branch is ready to merge!

@reyang reyang merged commit 5f4a464 into open-telemetry:master Jul 2, 2020
@nadiaciobanu nadiaciobanu deleted the basic-otlp-recordable branch July 23, 2020 22:27
@reyang reyang mentioned this pull request Aug 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZiweiZhao ZiweiZhao ZiweiZhao left review comments

@g-easy g-easy g-easy left review comments

@IlyaKobelevskiy IlyaKobelevskiy IlyaKobelevskiy approved these changes

@pyohannes pyohannes pyohannes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nadiaciobanu @ZiweiZhao @IlyaKobelevskiy @pyohannes @g-easy @reyang