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

feat: 🦠 Metrics prototype for Rack instrumentation #1129

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

Conversation

kaylareopelle
Copy link
Contributor

@kaylareopelle kaylareopelle commented Aug 19, 2024

Here's a prototype to add the http.server.request.duration metric to Rack instrumentation.

The base gem looks for the presence of the opentelemetry-metrics-api, by checking to see if OpenTelemetry::Metrics is defined before it creates a meter. Then, the instrumentation checks for the :send_metrics configuration to be true. If both conditions are true, metrics will be generated. Guard clauses are used to determine whether the metrics-related code should run.

In order to test this with an application, you need to patch the OpenTelemetry::SDK::Configurator#metrics_configuration_hook to add a meter provider and metric reader before configuring the SDK. Here's an example that uses the periodic metric reader and the OTLP metrics exporter:

module MetricsPatch
  def metrics_configuration_hook
    OpenTelemetry.meter_provider = OpenTelemetry::SDK::Metrics::MeterProvider.new(resource: @resource)
    OpenTelemetry.meter_provider
      .add_metric_reader(
        OpenTelemetry::SDK::Metrics::Export::PeriodicMetricReader.new(
          exporter: OpenTelemetry::Exporter::OTLP::Metrics::MetricsExporter.new,
          export_interval_millis: 3000,
          export_timeout_millis: 10000))
  end
end

OpenTelemetry::SDK::Configurator.prepend(MetricsPatch)

OpenTelemetry::SDK.configure do |c|
    c.use 'OpenTelemetry::Instrumentation::Rack', { send_metrics: true }
end

The attributes assigned to the http.server.request.duration metric are drawn from the span attributes. When OTEL_SEMCONV_STABILITY_OPT_IN is ready, we should update the instrumentation to emit the stability-related attributes for both spans and metrics.

To use this with another instrumentation that installs Rack (ex. Sinatra), you'll need to update the gemspec for the instrumentation that installs Rack so that the opentelemetry-instrumentation-base and opentelemetry-instrumentation-rack dependencies aren't installed.

Closes #1127

Emit http.server.request.duration metric if the application has the
metrics API installed and enables the feature via config
Otherwise it may round to zero
Like the Metrics ConfigurationPatch, include the MetricsPatch modules if
Metrics is enabled/present
@kaylareopelle kaylareopelle changed the title 🦠 Metrics prototype: Prepend edition WIP: 🦠 Metrics prototype: Prepend edition Sep 10, 2024
@kaylareopelle kaylareopelle changed the title WIP: 🦠 Metrics prototype: Prepend edition 🦠 Metrics prototype: Prepend edition Sep 10, 2024
@kaylareopelle kaylareopelle linked an issue Sep 20, 2024 that may be closed by this pull request
@kaylareopelle kaylareopelle changed the title 🦠 Metrics prototype: Prepend edition feat: 🦠 Metrics prototype: prepend edition Sep 20, 2024
@kaylareopelle kaylareopelle marked this pull request as ready for review September 20, 2024 16:28
@http_server_request_duration_histogram ||= meter.create_histogram('http.server.request.duration', unit: 's', description: 'Duration of HTTP server requests.')
end

def record_http_server_request_duration_metric(span)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is very long because it's leaving all the options open for attribute names. The attribute names we currently use are not the ones marked as stable.

I think we could use OTEL_SEMCONV_STABILITY_OPT_IN as a way to determine what attributes to send. Or, we could only emit the metric if that env var is set to http, which would mean only the new attributes would be sent. I'd love feedback here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the metrics attribute only cares about HTTP-related attributes, can it emit metrics with the attributes that are based on the existing span.attributes? I'm thinking something like:

# Assume only one of url.scheme and http.scheme will appear
span.attributes.select { |key, value| key.match(/http\..*/) || key.match(/url\..*/) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like where you're headed with this! There's a lot of duplication in the current code. However, there are some http.* and url.* attributes present on the span that should not be attached to metrics (ex. url.full is on spans, but not metrics).

I wonder if we could use a key-based allowlist to select things instead? Maybe add a constant with all the HTTP metrics keys, and we search for those?

Things might need to shift again when the OTEL_SEMCONV_STABILITY_OPT_IN variable is out in the world, but we can address that later.

@@ -7,6 +7,7 @@
require 'opentelemetry'
require 'opentelemetry-registry'
require 'opentelemetry/instrumentation/base'
require 'opentelemetry/instrumentation/metrics_patch' if defined?(OpenTelemetry::Metrics) # maybe also add Env var check?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked about requiring "two keys to be turned", to enable metrics on instrumentation:

  1. Detect the metrics api is installed by the application (don't list it as a dependency)
  2. Add a configuration option to send the metrics (turned off by default)

Since we do some of our setup in base, base checks to see if the library is installed. If it is installed, we require the patch. The config isn't available at this time, so we don't use it in this context. The instrumentation for a particular library, however, checks for both the installation of the API and that the config option is true.

Is this sufficient to protect non-metrics users from any potential problems? Are there other approaches we'd like to explore?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two safeguards are enough for users who don’t want metrics in instrumentation or wish to create custom metrics themselves (e.g., include the metrics SDK but not use it in instrumentation).

Is the patch (e.g., prepend) only because metrics are still experimental features? I was thinking of including them in the base but not installing if OpenTelemetry::Metrics is missing (similar to line 25 in metrics_patch.rb).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I think that could work! Yes, the patch is just because the metrics are experimental features. I thought this approach might feel "safer" (since all our instrumentation runs through base), but I don't know if truly provides a benefit.

It also tries to follow the configurator patches for metrics in the SDK:

I'm happy to cut out the patch and just put everything in base. I sorta tried that with the other prototype.

@kaylareopelle kaylareopelle self-assigned this Sep 27, 2024
@kaylareopelle kaylareopelle changed the title feat: 🦠 Metrics prototype: prepend edition feat: 🦠 Metrics prototype for Rack instrumentation Oct 3, 2024
@kaylareopelle
Copy link
Contributor Author

@xuan-cao-swi - Thank you for the feedback! I've updated the prototype to use conditions instead of patches and to add the attributes from the spans using select instead of adding each to a hash individually. Please take another look when you have a chance!

end

def record_http_server_request_duration_metric(span)
return unless metrics_enabled?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a bunch of ideas and questions floating around in my head that could use your input being a metrics SDK expert.

It may result in a separate dispatch call, but have you considered creating a separate handler for metric generation? That would remove the need for having conditionals in the tracer middleware.

What if our user has an o11y 1.0 mindset and only wanted to generate metrics and not traces? Would that not mean the span would be non_recording and the metric would always generate invalid histogram entries?

Is there anything in the spec that states this should be generated from span timestamps?

Is there an alternative implementation that generates metrics in the span processor pipeline?

E.g. there is a a processor that implements on_finish generated metrics for server and client spans regardless of the instrumentation?

What about one that decorates the BatchSpanProcessor export loop and generates metrics in the bsp thread to minimize the metrics from being generated in the critical path of the users request?

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.

Determine the structure for Metrics instrumentation
3 participants