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

Data points in exporter shouldnt use bound instruments #1237

Merged
merged 6 commits into from
Oct 31, 2020
Merged

Data points in exporter shouldnt use bound instruments #1237

merged 6 commits into from
Oct 31, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Oct 13, 2020 •
edited
Loading

Fixes #1236

Type of change

Please delete options that are not relevant.

  • 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 not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Updated translate_metrics test

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been updated
  • Documentation has been updated

@ocelotl ocelotl self-assigned this Oct 13, 2020
@ocelotl ocelotl force-pushed the issue_1236 branch 2 times, most recently from b17530e to 5026b9b Compare October 16, 2020 04:46
@ocelotl ocelotl marked this pull request as ready for review October 16, 2020 05:01
@ocelotl ocelotl requested review from a team, codeboten and hectorhdzg and removed request for a team October 16, 2020 05:01
@ocelotl ocelotl changed the title WIP: data points in exporter shouldnt use bound instruments Data points in exporter shouldnt use bound instruments Oct 16, 2020
lzchen
lzchen requested changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

There are several changes in the PR that are not listed in the description. Can you add CHANGELOG entries to otlp, system metrics and sdk packages to explain what is changed?

@ocelotl
Copy link
Contributor Author

ocelotl commented Oct 16, 2020

There are several changes in the PR that are not listed in the description. Can you add CHANGELOG entries to otlp, system metrics and sdk packages to explain what is changed?

Done 👍

@ocelotl ocelotl requested a review from lzchen October 16, 2020 19:55
codeboten
codeboten suggested changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Couple of questions I'd like answered before approving

instrumentation/opentelemetry-instrumentation-system-metrics/CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/controller.py Outdated Show resolved Hide resolved
@ocelotl ocelotl requested a review from codeboten October 16, 2020 21:44
@ocelotl ocelotl requested a review from codeboten October 19, 2020 22:09
@NathanielRN NathanielRN mentioned this pull request Oct 22, 2020
Closed
1 task
lzchen
lzchen requested changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

I would split this pr up into multiple prs if we are changing more behavior than what is outlined in the current issue.

@ocelotl ocelotl requested a review from lzchen October 28, 2020 15:31
@ocelotl ocelotl force-pushed the issue_1236 branch 2 times, most recently from 9a1dca2 to d9c4da6 Compare October 28, 2020 15:47
@ocelotl
Copy link
Contributor Author

ocelotl commented Oct 28, 2020

I would split this pr up into multiple prs if we are changing more behavior than what is outlined in the current issue.

I removed the wrong commits, this PR is as it was before now.

@lzchen lzchen assigned lzchen and unassigned ocelotl Oct 29, 2020
lzchen
lzchen approved these changes Oct 31, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM. If you can create an issue for the last_checkpoint_timestamp that'd be great.

@lzchen lzchen merged commit 7d9b0e5 into open-telemetry:master Oct 31, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@markwolff @mayurkale22
fix(package.json): publish source maps (open-telemetry#1237)
153169a 
Co-authored-by: Mayur Kale <[email protected]>
@ocelotl ocelotl mentioned this pull request Nov 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@lzchen lzchen lzchen approved these changes

@hectorhdzg hectorhdzg Awaiting requested review from hectorhdzg

Assignees

@lzchen lzchen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OTLP exporter uses bound_instruments
3 participants
@ocelotl @codeboten @lzchen