Use Rack compliant keys with deprecation #365
Conversation
| super | ||
| end | ||
|
|
||
| def []=(key, value) |
There was a problem hiding this comment.
the setter is required as a deprecation warning? we're not setting : datadog_rack_request_span anymore and if users do, it's not something we should not interfere I think
There was a problem hiding this comment.
Was doing this out of abundance of caution, in case someone was overriding this value. Seems very unlikely, but possible.
| env.instance_eval do | ||
| def [](key) | ||
| if key == :datadog_rack_request_span | ||
| Datadog::Tracer.log.warn(REQUEST_SPAN_DEPRECATION_WARNING) |
There was a problem hiding this comment.
This one could be very spammy. If that internal attribute is used in a middleware, it means 1 log for each request.
There was a problem hiding this comment.
Adding an instance variable to scope this to once per request. Our own code shouldn't ever generate these warnings.
| env[RACK_REQUEST_SPAN] = request_span | ||
|
|
||
| # For backwards compatibility; remove later. | ||
| env[:datadog_rack_request_span] = env[RACK_REQUEST_SPAN] |
There was a problem hiding this comment.
good to me, but let's add a TODO: just for the highlight. Also we should probably think (before shipping / merging this part) to write in the comment: remove later -> this attribute is deprecated and will be removed in 0.13 (it's just an example).
There was a problem hiding this comment.
Yeah, good idea. I will add in the "will be removed in 0.13" for now, and if we change the plan, we can adjust it later.
delner
force-pushed
the
deprecation/rack_compliant_keys
branch
from
651d184
to
a0b4571
Compare
|
Feedback should be addressed. |
| private | ||
|
|
||
| REQUEST_SPAN_DEPRECATION_WARNING = %( | ||
| Use of :datadog_rack_request_span in the Rack env is DEPRECATED. |
There was a problem hiding this comment.
I would say: ":datadog_rack_request_span is an internal symbol in the Rack env and is DEPRECATED. If you need (etc...)"
| REQUEST_SPAN_DEPRECATION_WARNING = %( | ||
| Use of :datadog_rack_request_span in the Rack env is DEPRECATED. | ||
| If you need the Rack request span, try using `Datadog.tracer.active_span`. | ||
| This key will be removed in version 0.13.0).freeze |
There was a problem hiding this comment.
We can think about it. We can introduce this change after at least 2-3 releases so people know that something is going to break because internal symbols (that unfortunately are public-facing) are used.
delner
force-pushed
the
deprecation/rack_compliant_keys
branch
from
a0b4571
to
7edd458
Compare
This pull request is a reissue of #338 , which fixed an issue with use of symbols with Rack, by converting a key to a string instead.
That pull request had to be reverted since it introduced a breaking change for anyone dependent on
env[:datadog_rack_request_span]. This pull request seeks to re-add those changes, but with backwards compatibility and a deprecation warning.In a future version, likely 0.13.0, we will remove both the deprecation warning and the backwards compatibility present here.