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.

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

OC Exporter - send start_timestamp, resource labels, and convert labels to strings #937

Merged
merged 9 commits into from
Aug 13, 2020

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jul 23, 2020

Fixes #940, fixes #951.

  1. Pass start_timestamp for stateful metrics
  2. Export resources as well. For resource type, copied logic from the Collector in Infer OpenCensus resource type based on OpenTelemetry's semantic conventions opentelemetry-collector#1462
  3. Convert any labels to str when setting it in proto. The labels should all be strings, but this is currently only type-checked.

Previously, start_timestamp wasn't being passed at all. For metrics that are cumulative over the life of the process, the exporter should be sending a start time since startup. The docstring for this start_timestamp:

// Must be present for cumulative metrics. The time when the cumulative value
// was reset to zero. Exclusive. The cumulative value is over the time interval
// (start_timestamp, timestamp]. If not specified, the backend can use the
// previous recorded value.
google.protobuf.Timestamp start_timestamp = 1;

The OpenCensus to OpenCensus Agent exporter sends the start_timestamp that is attached to each series, but OT doesn't store the start time in the aggregator or batcher AFAIK. I feel like this should be added, but we are waiting for the SDK specification still.

Questions

@aabmass aabmass force-pushed the fix-oc-exporter branch 2 times, most recently from 401037e to d06c3a7 Compare July 28, 2020 15:47
@aabmass aabmass changed the title pass start_timestamp to the timeseries in OC exporter Fixes for OpenCensusMetricsExporter Jul 28, 2020
@aabmass aabmass changed the title Fixes for OpenCensusMetricsExporter Send start_timestamp and convert labels to strings Jul 28, 2020
@aabmass aabmass force-pushed the fix-oc-exporter branch 2 times, most recently from 1866849 to e044ea8 Compare July 28, 2020 16:12
@aabmass aabmass marked this pull request as ready for review July 28, 2020 16:13
@aabmass aabmass requested a review from a team July 28, 2020 16:13
aabmass added 2 commits July 28, 2020 19:53
- Pass start_timestamp to the timeseries in OC exporter for stateful
metrics
- Convert label values to strings, just in case
@aabmass aabmass changed the title Send start_timestamp and convert labels to strings Send start_timestamp, resource labels, and convert labels to strings Jul 28, 2020
@lzchen
Copy link
Contributor

lzchen commented Jul 29, 2020

The OpenCensus to OpenCensus Agent exporter sends the start_timestamp that is attached to each series, but OT doesn't store the start time in the aggregator or batcher AFAIK. I feel like this should be added, but we are waiting for the SDK specification still.

What does "start time" mean? When the metric is recorded or exported? Does it only make sense for non-stateful instruments?

EDIT: From the specs: "The time when the cumulative valuewas reset to zero." So this leads me to believe that it is upon export time. Wouldn't the exporter be responsible for this value then instead of the metric/aggregator/batcher?

Should we also send the start_timestamp for non-stateful instruments

What would start_timestamp mean for non-stateful instruments? If following the specs, when their value was reset to zero would be upon every export right?

@aabmass
Copy link
Member Author

aabmass commented Jul 30, 2020

So this leads me to believe that it is upon export time. Wouldn't the exporter be responsible for this value then instead of the metric/aggregator/batcher?

By "upon export time" do you mean from whenever the exporter started up? That's what I'm sending for stateful metrics

What would start_timestamp mean for non-stateful instruments? If following the specs, when their value was reset to zero would be upon every export right?

That is my understanding, although the docstring in the proto file makes it sound likes this is done automatically: "If not specified, the backend can use the previous recorded value."

@lzchen
Copy link
Contributor

lzchen commented Jul 30, 2020

@aabmass

By "upon export time" do you mean from whenever the exporter started up?

Ahh sorry I misunderstood what you meant by start time. Forget this question.

but OT doesn't store the start time in the aggregator or batcher AFAIK

I asked this originally to address this. I'm not sure if starttime should be part of the aggregator/batcher? Wouldn't it make sense for it to be just part of the exporter when it is instantiated/first exported?

@aabmass
Copy link
Member Author

aabmass commented Aug 3, 2020

Wouldn't it make sense for it to be just part of the exporter when it is instantiated/first exported

@lzchen, I am going with when the exporter was instantiated for now. I think this is as accurate as we can get without adding a start_time to the SDK.

@ocelotl ocelotl self-requested a review August 6, 2020 23:41
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just minor comments

@aabmass aabmass changed the title Send start_timestamp, resource labels, and convert labels to strings OC Exporter - send start_timestamp, resource labels, and convert labels to strings Aug 11, 2020
@lzchen lzchen merged commit 3efe854 into open-telemetry:master Aug 13, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* fix: Jaeger propagator example of usage

* fix: Jaeger propagator example of usage
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.

OpenCensusMetricsExporter: GKE resource labels not sent OpenCensusExporter issues
4 participants