Skip to content
This repository was archived by the owner on Apr 19, 2026. It is now read-only.

Added Custom Recordable Implementation and Unit Tests#2

Merged
IlyaKobelevskiy merged 4 commits into
GoogleCloudPlatform:masterfrom
snehilchopra:gcp_recordable
Jul 2, 2020
Merged

Added Custom Recordable Implementation and Unit Tests#2
IlyaKobelevskiy merged 4 commits into
GoogleCloudPlatform:masterfrom
snehilchopra:gcp_recordable

Conversation

@snehilchopra
Copy link
Copy Markdown
Contributor

Added a custom Recordable implementation pertaining to the Google Cloudtrace V2 API Spans.

Attributes were recently added in the opentelemtry-cpp repo (Attributes PR), and accordingly I have implemented the SetAttribute method to handle them.

This will need to be updated gradually as more span data is added to the opentelemetry-cpp repo.

Copy link
Copy Markdown

@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.

That looks good and is true to the Recordable intention.

Comment thread exporters/trace/gcp_exporter/internal/recordable_test.cc Outdated
Comment thread exporters/trace/gcp_exporter/internal/recordable.cc Outdated
Comment thread exporters/trace/gcp_exporter/internal/recordable_test.cc Outdated
Comment thread exporters/trace/gcp_exporter/internal/recordable_test.cc Outdated
Comment thread exporters/trace/gcp_exporter/internal/recordable.cc
Comment thread exporters/trace/gcp_exporter/internal/recordable.cc Outdated
Copy link
Copy Markdown

@ZiweiZhao ZiweiZhao left a comment

Choose a reason for hiding this comment

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

Overall LGTM


const std::chrono::nanoseconds start_time_nanos(reinterpret_cast<int32_t>(rec.span().start_time().nanos()));
const std::chrono::seconds start_time_seconds(reinterpret_cast<int64_t>(rec.span().start_time().seconds()));
const std::chrono::nanoseconds unix_start_time(std::chrono::duration_cast<std::chrono::nanoseconds>(start_time_seconds).count()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: I'm not sure if OpenTel has a specification on line length, if there is not, I would suggest 80 chars per line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll try adjusting to 80 chars wherever I can.
But in some places, I believe this adjustment might affect the readability.

@IlyaKobelevskiy IlyaKobelevskiy merged commit 5d8da0a into GoogleCloudPlatform:master Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants