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

core: ensure http.status_code tag is always a string #927

Merged
merged 4 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/ddtrace/span.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ def set_tag(key, value = nil)
# Keys must be unique between tags and metrics
@metrics.delete(key)

# Ensure `http.status_code` is always a string so it is added to
# @meta instead of @metrics
# DEV: This is necessary because the agent looks to `meta['http.status_code']` for
# tagging necessary metrics
value = value.to_s if key == Ext::HTTP::STATUS_CODE

# NOTE: Adding numeric tags as metrics is stop-gap support
# for numeric typed tags. Eventually they will become
# tags again.
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/contrib/aws/instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
expect(span.get_tag('path')).to eq('/')
expect(span.get_tag('host')).to eq('s3.us-stubbed-1.amazonaws.com')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
end

it 'returns the correct response' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/elasticsearch/transport_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def call(env)
expect(span.resource).to eq('GET _cluster/health')
expect(span.get_tag('elasticsearch.url')).to eq('_cluster/health')
expect(span.get_tag('elasticsearch.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('elasticsearch.params')).to be nil
expect(span.get_tag('elasticsearch.body')).to be nil
expect(span.get_tag('out.host')).to eq(host)
Expand Down Expand Up @@ -111,7 +111,7 @@ def call(env)
expect(span.resource).to eq('PUT my/thing/?')
expect(span.get_tag('elasticsearch.url')).to eq(path)
expect(span.get_tag('elasticsearch.method')).to eq('PUT')
expect(span.get_tag('http.status_code')).to eq(201)
expect(span.get_tag('http.status_code')).to eq('201')
expect(span.get_tag('elasticsearch.params')).to eq(params.to_json)
expect(span.get_tag('elasticsearch.body')).to eq('{"data1":"?","data2":"?"}')
expect(span.get_tag('out.host')).to eq(host)
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/ethon/easy_patch_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@
end

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(500)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('500')
end

it 'has error set' do
Expand All @@ -127,7 +127,7 @@
end

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(404)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('404')
end

it 'has no error set' do
Expand Down
6 changes: 3 additions & 3 deletions spec/ddtrace/contrib/ethon/shared_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
end

it 'has tag with status code' do
expected_status = status ? status.to_f : nil
expected_status = status ? status.to_s : nil
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(expected_status)
end

Expand Down Expand Up @@ -68,7 +68,7 @@
before { request }

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_f)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_s)
end

it 'has error set' do
Expand All @@ -88,7 +88,7 @@
before { request }

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_f)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_s)
end

it 'has no error set' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/excon/instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
expect(request_span.name).to eq(Datadog::Contrib::Excon::Ext::SPAN_REQUEST)
expect(request_span.resource).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::METHOD)).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(200)
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('200')
expect(request_span.get_tag(Datadog::Ext::HTTP::URL)).to eq('/success')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_HOST)).to eq('example.com')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_PORT)).to eq(80)
Expand All @@ -109,7 +109,7 @@
expect(request_span.resource).to eq('POST')
expect(request_span.get_tag(Datadog::Ext::HTTP::METHOD)).to eq('POST')
expect(request_span.get_tag(Datadog::Ext::HTTP::URL)).to eq('/failure')
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(500)
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('500')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_HOST)).to eq('example.com')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_PORT)).to eq(80)
expect(request_span.span_type).to eq(Datadog::Ext::HTTP::TYPE_OUTBOUND)
Expand Down
6 changes: 3 additions & 3 deletions spec/ddtrace/contrib/faraday/middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
expect(request_span.name).to eq(Datadog::Contrib::Faraday::Ext::SPAN_REQUEST)
expect(request_span.resource).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::METHOD)).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(200)
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('200')
expect(request_span.get_tag(Datadog::Ext::HTTP::URL)).to eq('/success')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_HOST)).to eq('example.com')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_PORT)).to eq(80)
Expand Down Expand Up @@ -87,7 +87,7 @@
expect(request_span.name).to eq(Datadog::Contrib::Faraday::Ext::SPAN_REQUEST)
expect(request_span.resource).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::METHOD)).to eq('GET')
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(200)
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('200')
expect(request_span.get_tag(Datadog::Ext::HTTP::URL)).to eq('/success')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_HOST)).to eq('example.com')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_PORT)).to eq(80)
Expand All @@ -105,7 +105,7 @@
expect(request_span.resource).to eq('POST')
expect(request_span.get_tag(Datadog::Ext::HTTP::METHOD)).to eq('POST')
expect(request_span.get_tag(Datadog::Ext::HTTP::URL)).to eq('/failure')
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(500)
expect(request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq('500')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_HOST)).to eq('example.com')
expect(request_span.get_tag(Datadog::Ext::NET::TARGET_PORT)).to eq(80)
expect(request_span.span_type).to eq(Datadog::Ext::HTTP::TYPE_OUTBOUND)
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/rack/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
expect(rack_span.service).to eq(Datadog.configuration[:rack][:service_name])
expect(rack_span.resource).to eq('GET 200')
expect(rack_span.get_tag('http.method')).to eq('GET')
expect(rack_span.get_tag('http.status_code')).to eq(200)
expect(rack_span.get_tag('http.status_code')).to eq('200')
expect(rack_span.get_tag('http.url')).to eq('/')
expect(rack_span.status).to eq(0)

Expand All @@ -103,7 +103,7 @@
expect(span.service).to eq(Datadog.configuration[:rack][:service_name])
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/')
expect(span.status).to eq(0)

Expand Down
28 changes: 14 additions & 14 deletions spec/ddtrace/contrib/rack/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 404')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(404)
expect(span.get_tag('http.status_code')).to eq('404')
expect(span.get_tag('http.url')).to eq('/not/exists/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand Down Expand Up @@ -90,7 +90,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/success/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand All @@ -107,7 +107,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
# Since REQUEST_URI isn't available in Rack::Test by default (comes from WEBrick/Puma)
# it reverts to PATH_INFO, which doesn't have query string parameters.
expect(span.get_tag('http.url')).to eq('/success')
Expand All @@ -129,7 +129,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
# Since REQUEST_URI is set (usually provided by WEBrick/Puma)
# it uses REQUEST_URI, which has query string parameters.
# However, that query string will be quantized.
Expand All @@ -149,7 +149,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
# Since REQUEST_URI is set (usually provided by WEBrick/Puma)
# it uses REQUEST_URI, which has query string parameters.
# The query string will not be quantized, per the option.
Expand All @@ -170,7 +170,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/success/100')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand All @@ -195,7 +195,7 @@
expect(span.service).to eq('custom-rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/success/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand All @@ -216,7 +216,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('POST 200')
expect(span.get_tag('http.method')).to eq('POST')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/success/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand Down Expand Up @@ -249,7 +249,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 400')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(400)
expect(span.get_tag('http.status_code')).to eq('400')
expect(span.get_tag('http.url')).to eq('/failure/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand Down Expand Up @@ -281,7 +281,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 500')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(500)
expect(span.get_tag('http.status_code')).to eq('500')
expect(span.get_tag('http.url')).to eq('/server_error/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.get_tag('error.stack')).to be nil
Expand Down Expand Up @@ -400,7 +400,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET /app/')
expect(span.get_tag('http.method')).to eq('GET_V2')
expect(span.get_tag('http.status_code')).to eq(201)
expect(span.get_tag('http.status_code')).to eq('201')
expect(span.get_tag('http.url')).to eq('/app/static/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand Down Expand Up @@ -442,7 +442,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 500')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(500)
expect(span.get_tag('http.status_code')).to eq('500')
expect(span.get_tag('http.url')).to eq('/app/500/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.get_tag('error.stack')).to eq('Handled exception')
Expand Down Expand Up @@ -477,7 +477,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 500')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(500)
expect(span.get_tag('http.status_code')).to eq('500')
expect(span.get_tag('http.url')).to eq('/app/500/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.get_tag('error.stack')).to eq('Handled exception')
Expand Down Expand Up @@ -600,7 +600,7 @@
expect(span.service).to eq('rack')
expect(span.resource).to eq('GET 200')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(200)
expect(span.get_tag('http.status_code')).to eq('200')
expect(span.get_tag('http.url')).to eq('/headers/')
expect(span.get_tag('http.base_url')).to eq('http://example.org')
expect(span.status).to eq(0)
Expand Down
8 changes: 4 additions & 4 deletions spec/ddtrace/contrib/rails/middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def call(env)
expect(span.resource).to eq('TestController#index')
expect(span.get_tag('http.url')).to eq('/')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(500)
expect(span.get_tag('http.status_code')).to eq('500')
expect(span.get_tag('error.type')).to eq('NotImplementedError')
expect(span.get_tag('error.msg')).to eq('NotImplementedError')
expect(span).to have_error
Expand Down Expand Up @@ -175,7 +175,7 @@ def call(env)
expect(span.resource).to eq('TestController#index')
expect(span.get_tag('http.url')).to eq('/')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(404)
expect(span.get_tag('http.status_code')).to eq('404')

if Rails.version >= '3.2'
expect(span.get_tag('error.type')).to be nil
Expand Down Expand Up @@ -238,7 +238,7 @@ def call(env)
expect(span.get_tag('http.url')).to eq('/') if Rails.version >= '3.2'

expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(500)
expect(span.get_tag('http.status_code')).to eq('500')
expect(span.get_tag('error.type')).to eq('CustomError')
expect(span.get_tag('error.msg')).to eq('Custom error message!')
expect(span).to have_error
Expand Down Expand Up @@ -283,7 +283,7 @@ def call(env)
expect(span.resource).to eq('TestController#index')
expect(span.get_tag('http.url')).to eq('/')
expect(span.get_tag('http.method')).to eq('GET')
expect(span.get_tag('http.status_code')).to eq(404)
expect(span.get_tag('http.status_code')).to eq('404')
expect(span.get_tag('error.type')).to be nil
expect(span.get_tag('error.msg')).to be nil
expect(span).to_not have_error
Expand Down
6 changes: 3 additions & 3 deletions spec/ddtrace/contrib/rest_client/request_patch_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
end

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_s)
end

it 'is http type' do
Expand Down Expand Up @@ -100,7 +100,7 @@
end

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_s)
end

it 'has error set' do
Expand All @@ -122,7 +122,7 @@
end

it 'has tag with status code' do
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status)
expect(span.get_tag(Datadog::Ext::HTTP::STATUS_CODE)).to eq(status.to_s)
end

it 'error is not set' do
Expand Down
9 changes: 8 additions & 1 deletion spec/ddtrace/span_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,17 @@
end
end

context 'given a numeric tag' do
context 'given http.status_code' do
let(:key) { 'http.status_code' }
let(:value) { 200 }

it_behaves_like 'meta tag'
end

context 'given a numeric tag' do
let(:key) { 'system.pid' }
let(:value) { 123 }

context 'which is an integer' do
context 'that exceeds the upper limit' do
let(:value) { described_class::NUMERIC_TAG_SIZE_RANGE.max.to_i + 1 }
Expand Down
Loading