-
Notifications
You must be signed in to change notification settings - Fork 380
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
Roda Integration #2368
Roda Integration #2368
Changes from all commits
de8db34
3d03eda
910d7fe
9114586
403831a
c8953fd
352d1cb
68d3b5d
72553ad
ab7d7fe
6103e1f
fef14ff
76c4a38
586fd57
5016c7b
4c74c4b
f8c0c32
1462d1e
aa1db97
d16dbd5
a9db21c
2342811
41be369
585b862
affbc7a
4f107a0
3061065
fe099ac
af40cb6
c3804ba
edb4f2b
4555d3d
0aff08c
901297b
8510779
426f6cf
78237cc
c66542d
b3081e9
9323c6c
e18b260
034eef6
c3a1e2c
5aa0e94
29a9aa8
0b0fe25
3682891
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative '../../configuration/settings' | ||
require_relative '../ext' | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module Roda | ||
module Configuration | ||
# Custom settings for the Roda integration | ||
class Settings < Contrib::Configuration::Settings | ||
option :enabled do |o| | ||
o.default { env_to_bool(Ext::ENV_ENABLED, true) } | ||
o.lazy | ||
end | ||
|
||
option :analytics_enabled do |o| | ||
o.default { env_to_bool(Ext::ENV_ANALYTICS_ENABLED, false) } | ||
o.lazy | ||
end | ||
|
||
option :analytics_sample_rate do |o| | ||
o.default { env_to_float(Ext::ENV_ANALYTICS_SAMPLE_RATE, 1.0) } | ||
o.lazy | ||
end | ||
|
||
option :service_name | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# frozen_string_literal: true | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module Roda | ||
# Roda integration constants | ||
module Ext | ||
APP = 'roda' | ||
ENV_ENABLED = 'DD_TRACE_RODA_ENABLED' | ||
ENV_ANALYTICS_ENABLED = 'DD_RODA_ANALYTICS_ENABLED' | ||
ENV_ANALYTICS_SAMPLE_RATE = 'DD_RODA_ANALYTICS_SAMPLE_RATE' | ||
SPAN_REQUEST = 'roda.request' | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative '../../metadata/ext' | ||
require_relative 'ext' | ||
require_relative '../analytics' | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module Roda | ||
# Instrumentation for Roda | ||
module Instrumentation | ||
def _roda_handle_main_route | ||
instrument(Ext::SPAN_REQUEST) { super } | ||
end | ||
|
||
def call | ||
instrument(Ext::SPAN_REQUEST) { super } | ||
end | ||
|
||
private | ||
|
||
def instrument(span_name, &block) | ||
Tracing.trace(span_name) do |span| | ||
begin | ||
request_method = request.request_method.to_s.upcase | ||
|
||
span.service = configuration[:service_name] if configuration[:service_name] | ||
|
||
span.span_type = Tracing::Metadata::Ext::HTTP::TYPE_INBOUND | ||
|
||
# Using the http method as a resource, since the URL/path can trigger | ||
# a possibly infinite number of resources. | ||
span.resource = request_method | ||
|
||
span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_URL, request.path) | ||
span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_METHOD, request_method) | ||
|
||
# Add analytics tag to the span | ||
if Contrib::Analytics.enabled?(configuration[:analytics_enabled]) | ||
Contrib::Analytics.set_sample_rate(span, configuration[:analytics_sample_rate]) | ||
end | ||
|
||
# Measure service stats | ||
Contrib::Analytics.set_measured(span) | ||
ensure | ||
begin | ||
response = yield | ||
rescue StandardError | ||
# The status code is unknown to Roda and decided by the upstream web runner. | ||
# In this case, spans default to status code 500 rather than a blank status code. | ||
default_error_status = '500' | ||
span.resource = "#{request_method} #{default_error_status}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly strange edge case here where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how this error can be thrown because all routes should be defined with a type from the start. Do you have a suggestion for handling this scenario? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I paired with Wan and it's not possible for |
||
span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_STATUS_CODE, default_error_status) | ||
raise | ||
end | ||
end | ||
|
||
status_code = response[0] | ||
|
||
# Adds status code to the resource name once the resource comes back | ||
span.resource = "#{request_method} #{status_code}" | ||
marcotc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_STATUS_CODE, status_code) | ||
span.status = 1 if status_code.to_s.start_with?('5') | ||
response | ||
end | ||
end | ||
|
||
def configuration | ||
Datadog.configuration.tracing[:roda] | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# frozen_string_literal: true | ||
|
||
# typed: false | ||
|
||
require_relative '../integration' | ||
require_relative 'configuration/settings' | ||
require_relative 'patcher' | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module Roda | ||
# Description of Roda integration | ||
class Integration | ||
include Contrib::Integration | ||
|
||
MINIMUM_VERSION = Gem::Version.new('2.0.0') | ||
MAXIMUM_VERSION = Gem::Version.new('4.0.0') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know this maximum. Do we know we don't support Roda 4.0.0? If so, why don't we? Also "maximum" version as a name is slightly misleading in that its exclusive (we don't actually support 4.0.0.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the time of this comment: Roda 4.x isn't out. The latest version is 3.65.0. Given the changes that occurred between 1.x and 2.x, I'm just being cautious by setting a maximum of 4.x for now. That way, in our public docs, we can be clear that this logic is intended for 2.x and 3.x. Whenever 4.x comes out, it'll require visiting to make sure it still functions as expected. |
||
|
||
register_as :roda | ||
|
||
def self.version | ||
Gem.loaded_specs['roda'] && Gem.loaded_specs['roda'].version | ||
end | ||
|
||
def self.loaded? | ||
!defined?(::Roda).nil? | ||
end | ||
|
||
def self.compatible? | ||
super && version >= MINIMUM_VERSION && version < MAXIMUM_VERSION | ||
end | ||
|
||
def new_configuration | ||
Configuration::Settings.new | ||
end | ||
|
||
def patcher | ||
Patcher | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# frozen_string_literal: true | ||
|
||
# typed: ignore | ||
|
||
require_relative 'patcher' | ||
require_relative 'ext' | ||
require_relative 'instrumentation' | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module Roda | ||
# Patcher enables patching of 'roda' | ||
module Patcher | ||
include Contrib::Patcher | ||
|
||
module_function | ||
|
||
def target_version | ||
Integration.version | ||
end | ||
|
||
def patch | ||
::Roda.prepend(Instrumentation) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
# typed: false | ||
|
||
require_relative './shared_examples' | ||
require 'roda' | ||
require 'ddtrace' | ||
require 'datadog/tracing/contrib/roda/instrumentation' | ||
require 'datadog/tracing/contrib/roda/ext' | ||
require 'datadog/tracing/contrib/support/spec_helper' | ||
require 'datadog/tracing/contrib/support/tracer_helpers' | ||
|
||
RSpec.describe Datadog::Tracing::Contrib::Roda::Instrumentation do | ||
describe 'when implemented in Roda' do | ||
let(:configuration_options) { {} } | ||
|
||
before do | ||
Datadog.configure do |c| | ||
c.tracing.instrument :roda, configuration_options | ||
end | ||
end | ||
|
||
after do | ||
Datadog.registry[:roda].reset_configuration! | ||
end | ||
|
||
describe 'When using automatic instrumentation' do | ||
let(:env) { { REQUEST_METHOD: 'GET' } } | ||
context 'configuring roda' do | ||
context 'with default settings' do | ||
it 'enables the tracer' do | ||
expect(Datadog.configuration.tracing.enabled).to eq(true) | ||
end | ||
|
||
it 'does not have a default service name (left up to global configuration)' do | ||
expect(Datadog.configuration.tracing[:roda].service_name).to eq(nil) | ||
expect(Datadog.configuration.service).to eq('rspec') | ||
end | ||
|
||
context 'with a custom service name' do | ||
let(:custom_service_name) { 'custom_service_name' } | ||
let(:configuration_options) { { service_name: custom_service_name } } | ||
|
||
it 'sets a custom service name' do | ||
expect(Datadog.configuration.service).to eq('rspec') | ||
expect(Datadog.configuration.tracing[:roda].service_name).to eq(custom_service_name) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
|
||
describe 'when application calls on the instrumented method' do | ||
context 'using #call' do | ||
it_behaves_like 'shared examples for roda', :call | ||
end | ||
|
||
context 'using #_roda_handle_main_route' do | ||
it_behaves_like 'shared examples for roda', :_roda_handle_main_route | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can move the error handling logic to the caller
So you could be confident yielding in the block without nested begin/rescue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in the Roda code, dispatch can land on one of these methods (seen in this conditional)
Would I need to duplicate the rescue block and handle the error in both since either one of these can handle the dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an update to this, I wasn't able to move the error handling logic anywhere else because it interfered with the errors produced by the underlying app.