Skip to content

Prefer Traces.trace. #124

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 19, 2023
Merged

Prefer Traces.trace. #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion async-http.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Gem::Specification.new do |spec|
spec.add_dependency "protocol-http", "~> 0.24.0"
spec.add_dependency "protocol-http1", "~> 0.15.0"
spec.add_dependency "protocol-http2", "~> 0.15.0"
spec.add_dependency "traces", ">= 0.8.0"
spec.add_dependency "traces", ">= 0.10.0"
Copy link

@dentarg dentarg Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Adding this dependency now mean async-http needs at least Ruby 2.7 to run but that is not reflected in the gemspec. https://github.com/socketry/traces/pull/8/files#r1237988182

Noticed this because Sinatra CI failed on Ruby 2.6: https://github.com/sinatra/sinatra/actions/runs/5263567184/jobs/9631906714#step:5:15

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks for the report. I have a general policy of not supporting EOL Rubies. However, we can probably drop 2.7 as a requirement for traces in the next release because we won't need to use ... any more. Hopefully I'll be able to do the next release in about a week.

Copy link

@dentarg dentarg Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, I've worked around the problem in Sinatra CI but I imagine it could be more difficult to do that in other projects (applications).

Just curious, should you be specifying the mimimum supported Ruby version in the gemspec for your gems? Then users of old Ruby versions can just keep using old versions of the gems (bundler should just pick the correct version for them).

Copy link
Member Author

@ioquatix ioquatix Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I usually try to do so, I'll probably need to automate it, I automate a lot of maintenance but don't always remember to fix the minimum version because it's not automated. I can describe the policy (drop EOL ruby), automate it, and then it should be easier. https://github.com/ioquatix/bake-modernize is my automation tool.


spec.add_development_dependency "async-container", "~> 0.14"
spec.add_development_dependency "async-rspec", "~> 1.10"
Expand Down
4 changes: 2 additions & 2 deletions lib/async/http/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ def call(request)
attributes['http.request.length'] = length
end

trace('async.http.client.call', attributes: attributes) do |span|
if context = self.trace_context
Traces.trace('async.http.client.call', attributes: attributes) do |span|
if context = Traces.trace_context
request.headers['traceparent'] = context.to_s
# request.headers['tracestate'] = context.state
end
Expand Down
4 changes: 2 additions & 2 deletions lib/async/http/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def run
Traces::Provider(self) do
def call(request)
if trace_parent = request.headers['traceparent']
self.trace_context = Traces::Context.parse(trace_parent.join, request.headers['tracestate'], remote: true)
Traces.trace_context = Traces::Context.parse(trace_parent.join, request.headers['tracestate'], remote: true)
end

attributes = {
Expand All @@ -80,7 +80,7 @@ def call(request)
attributes['http.protocol'] = protocol
end

trace('async.http.server.call', resource: "#{request.method} #{request.path}", attributes: attributes) do |span|
Traces.trace('async.http.server.call', resource: "#{request.method} #{request.path}", attributes: attributes) do |span|
super.tap do |response|
if status = response&.status
span['http.status_code'] = status
Expand Down