Bump OpenTelemetry dependency to v0.9.0 #73
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Oh no, I forgot to take into account the CLA requirement... |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Need a fix to ensure export type is correctly specified as Cumulative (see comments below). Also, it would be great if you could validate this works by following the example in the example/metric package as I'm not 100% confident.
Ideally, we would have an integration test that verifies we are exporting this data correctly. FYI @mabdi3 - will you be adding a test case that checks the values emitted are correct?
i.e.
- counter.Add(1)
- export (DELTA = 1, CUMULATIVE = 1, validate exported value is 1)
- counter.Add(1)
- export (DELTA = 1, CUMULATIVE = 2, validate exported value is 2)
| @@ -186,7 +187,7 @@ func (me *metricExporter) ExportMetrics(ctx context.Context, cps export.Checkpoi | |||
| // if the descriptor is not registered in Cloud Monitoring yet. | |||
| func (me *metricExporter) exportMetricDescriptor(ctx context.Context, cps export.CheckpointSet) error { | |||
| mds := make(map[key]*googlemetricpb.MetricDescriptor) | |||
| aggError := cps.ForEach(func(r export.Record) error { | |||
| aggError := cps.ForEach(export.CumulativeExporter, func(r export.Record) error { | |||
There was a problem hiding this comment.
Technically we always export Cumulatives, but this is not how the interfaces are intended to be used. The interface design here is a little odd / confusing and I wouldn't be surprised if this changes before GA.
The first argument to ForEach is an ExportSelector. You should pass through me itself here as the exporter already implements this interface, rather than passing in this constant selector (likewise for each of the calls to ForEach below). This will mean the same function mentioned above will be used, rather than us having to specify Cumulative in multiple places here.
There was a problem hiding this comment.
Hm, it doesn't seem to think metricExporter implements the ExportKindSelector interface:
../../exporter/metric/metric.go:190:26: cannot use *me (type metricExporter) as type "go.opentelemetry.io/otel/sdk/export/metric".ExportKindSelector in argument to cps.ForEach:
metricExporter does not implement "go.opentelemetry.io/otel/sdk/export/metric".ExportKindSelector (missing ExportKindFor method)
There was a problem hiding this comment.
Hmm yea this is a bit weird because we have the Export wrapper. You implemented the ExportKindSelector above (https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/73/files#diff-61301ca921134b70b15b20369de304f1R90) but its for the Exporter type rather than the metricExporter type
This might warrant some more significant refactoring but for now you could just pass the wrapper exporter into the newMetricExporter constructor so that you can use it here.
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
No, there weren't really any plans for this. The integrations tests weren't really to test the exporters themselves but that the mock api behind the calls were executing successfully. I'm not very knowledgeable on the exporter logic, so if this is a needed test, we can talk offline about how that might look. Also, if you want, tests can be added whenever. All one must do is make sure to point the client endpoint to our mock api. See here for an example. If you have any questions, I'm happy to chat. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
How is this PR going? @gzhao408 Given that we only export Cumulatives and this part of the SDK is likely to change, I'm happy with leaving this PR as is where you specify CUMULATIVE everywhere for now. However, you will need to rebase and run @marvinkite Looking at the CLA checker, it appears this is failing because you have not associated your Github email address with your commit. The CLA needs to verify that the e-mail address associated with every commit matches an e-mail address that has signed the CLA. Are you okay sharing this e-mail address with us so we can attach it to the commit? (currently it is associated with an anonymous Github email) |
|
@james-bebbington I use anonymous email but the CLA contains my GitHub username. I don't wish to share the email in public but I give my consent to use my work without mentioning my name at all so @gzhao408 can squash it without my name. |
Ok great. Thankyou |
|
@james-bebbington yes sorry, planning on getting back to this and applying the changes you suggested, but ran into a conflict that I wasn't sure how to fix while I was rebasing. We have a sprint ending today but I'll try to get back to this asap. I can also edit the authorship while I do that, thanks @marvinkite. |
All good, just wanted to make sure you weren't blocked |
original author @marvinkite consented to be removed from authorship see PR#73 conversation
|
@james-bebbington just pushed an update, it's nothing fancy, but I don't have a very good understanding of the codebase unfortunately :( |
There was a problem hiding this comment.
One minor change left.
I also created #78 to track updating this properly in the future.
|
Looks like there are more breaking changes in v0.10.0: #77 |
Ulterior Motive: I need this for stackdriver-sandbox instrumentation :). I need v0.9.0 for what I'm doing but it isn't compatible with the metrics exporter.
Most of this is picked up from #57, I merely rebased and squashed, left marvinkite as the author. There was some discussion on exporter kinds so I changed it to CumulativeExporter, but not 100% sure as this is above my head...