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

docs(sdk-metrics): add example of exponential histogram metric #3855

Conversation

JamieDanielson
Copy link
Member

Which problem is this PR solving?

Fixes #3852

Short description of the changes

  • Adds example of Exponential Histogram metric to existing metric example app.
  • Also adds a (commented-out) ConsoleMetricExporter for quick dev testing
  • Updates a few typos while I was in the area

I didn't update the metrics.md with the specific example because that didn't seem to have all the specific examples; I will look to update the public website next though (opentelemetry.io)

Type of change

  • Other; this is a documentation/example update

How Has This Been Tested?

  • Run the example app with the ConsoleMetricExporter (comment out or don't use the OTLPMetricExporter) and see the example metric in the console. Run with debug to see all the extra datapoints
{
  descriptor: {
    name: 'test_exponential_histogram',
    description: 'Example of an ExponentialHistogram',
    type: 'HISTOGRAM',
    unit: '',
    valueType: 1
  },
  dataPointType: 1,
  dataPoints: [
    {
      attributes: [Object],
      startTime: [Array],
      endTime: [Array],
      value: [Object]
    }
  ]
}

exponential-histogram-datapoints

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added (N/A)
  • Documentation has been updated (N/A)

@JamieDanielson JamieDanielson requested a review from a team June 2, 2023 19:02
@JamieDanielson JamieDanielson self-assigned this Jun 2, 2023
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #3855 (ee1b9d3) into main (6d13eb4) will not change coverage.
The diff coverage is n/a.

❗ Current head ee1b9d3 differs from pull request most recent head 8434869. Consider uploading reports for the commit 8434869 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3855   +/-   ##
=======================================
  Coverage   92.32%   92.32%           
=======================================
  Files         321      321           
  Lines        9167     9167           
  Branches     1945     1945           
=======================================
  Hits         8463     8463           
  Misses        704      704           

@JamieDanielson JamieDanielson changed the title docs: add example of exponential histogram metric docs(sdk-metrics): add example of exponential histogram metric Jun 2, 2023
@JamieDanielson JamieDanielson force-pushed the jamie.example-exponential-histogram branch from 8ba4dfe to fefedbd Compare June 2, 2023 19:35
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.

It's probably worth documenting that you can also achieve this for all histograms by setting this field on the metric reader:

aggregationSelector?: AggregationSelector;

There is also an environment variable for this OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION but it doesn't look like it has been implemented yet (opened #3920)

@JamieDanielson
Copy link
Member Author

Sorry all, I totally did a drive-by on this and forgot to come back to it. I committed the suggested change (forgot I changed that before pushing 😅 thank you).
I'll take a look at this again tomorrow and update accordingly.

@pichlermarc pichlermarc added the document Documentation-related label Jun 22, 2023
@JamieDanielson
Copy link
Member Author

Sorry for the slowness on this one! If this is good enough maybe we merge it in as-is and follow up with a better example/docs (along with the new env var implementation). I admittedly don't have a ton of experience with histograms 😅 so that is definitely adding to the bare bones of the example; was just hoping to get a little something in an example app.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for adding this to the example 🙂

examples/otlp-exporter-node/metrics.js Outdated Show resolved Hide resolved
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

This is great, thanks 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How do we create an Exponential Histogram
4 participants