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

Measure http server durations in seconds #1946

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gouthamve
Copy link
Member

See: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration

I couldn't figure out how to set the histograms' buckets though :(

Description

The unit for http.server.request.duration should be seconds and not milliseconds.

Type of change

Please delete options that are not relevant.

  • 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?

This has not been tested yet. I want to start the discussion around this before doing more work.

  • Test A

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

See: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration

I couldn't figure out how to set the histograms' buckets though :(

Signed-off-by: gouthamve <gouthamve@gmail.com>
@srikanthccv
Copy link
Member

I couldn't figure out how to set the histograms' buckets though :(

The advisory API has not been implemented yet.

This has not been tested yet. I want to start the discussion around this before doing more work.

There are existing tests for each instrumentation that should be updated, ex:

start = default_timer()
self.client.post("/hello/756")
self.client.post("/hello/756")
self.client.post("/hello/756")
duration = max(round((default_timer() - start) * 1000), 0)
metrics_list = self.memory_metrics_reader.get_metrics_data()
for resource_metric in metrics_list.resource_metrics:
for scope_metric in resource_metric.scope_metrics:
for metric in scope_metric.metrics:
for point in list(metric.data.data_points):
if isinstance(point, HistogramDataPoint):
self.assertEqual(point.count, 3)
self.assertAlmostEqual(
duration, point.sum, delta=10
)
Or did you want to discuss something else?

@lzchen I belive this also comes under the transition plan for HTTP semantic conventions update #1768?

@ocelotl
Copy link
Contributor

ocelotl commented Oct 2, 2023

Please add a CHANGELOG entry.

@lzchen
Copy link
Contributor

lzchen commented Oct 12, 2023

@srikanthccv

Based off of this decision, any breaking changes made while upgrading semantic conventions that conflict, we will take the new conventions as priority. The only exceptions to this would be attributes, and we will be implementing the migration plan for those. So this change in particular does not need to do anything special in terms of migration.

@lzchen
Copy link
Contributor

lzchen commented Oct 12, 2023

Part of this tracking issue: #1999

@ocelotl ocelotl removed their assignment Sep 3, 2024
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.

5 participants