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

Don't collect empty metrics from storage #3395

Closed

Conversation

spenczar
Copy link

Description

Without doing this, the pb_metric2.Metric has no value for the oneof data field, which means it does not have any known type. This breaks many exporters. Instead, when faced with no data points for a metric, we should present that empty dataset.

Fixes #3277.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have tested this a little with tox, but I haven't written thorough tests. I'm not sure where I'd put those - this is a big, complicated repo, and writing good tests is a challenge.

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

If a metric has no datapoints, don't send it on. That can
foul up exporters.
@spenczar spenczar requested a review from a team July 28, 2023 20:31
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: spenczar / name: Spencer Nelson (056d3bb)
  • ✅ login: ocelotl / name: Diego Hurtado (cf0492b)

@srikanthccv
Copy link
Member

This has some overlap with this PR #3335. I think the concerns expressed are valid. @aabmass was there any SIG discussion on it?

@aabmass
Copy link
Member

aabmass commented Aug 3, 2023

Let's discuss it in next week's SIG. I agree there is some overlap here.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

I think this is an SDK bug and the exporter shouldn't have to handle the special case. Lets see if we can fix it in the SDK instead.

@ocelotl
Copy link
Contributor

ocelotl commented Aug 10, 2023

@aabmass brought this topic in the SIG, please leave here any opinions you may have.

@ocelotl
Copy link
Contributor

ocelotl commented Sep 8, 2023

Superseded by #3335, closing

@ocelotl ocelotl closed this Sep 8, 2023
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.

Histograms are exported empty if no observations are recorded during a reading interval
4 participants