Pass omit_default_port from request to response

[mburumaxwell]

Similar to #803, this PR passes the omit_default_port attribute to the response.

Why?
When subscribed via ActiveNotifications, the Url formed for the request is not similar to the one for the response. The later includes the port. E.g. https://contoso.com:443/my-path

The URLs are generated using Excon::Utils.request_uri(...)

The code:

ActiveSupport::Notifications.subscribe(/excon/) do |*args|
   name = args.first
   $network_trace_count += 1 if name == "excon.request"

   payload = args.last
   if name == "excon.request" || name == "excon.response"
     puts "🌍 #{name == 'excon.response' ? "<-- #{payload[:status]}" : "--> #{payload[:method].upcase}"}" \
          " #{Excon::Utils.request_uri(payload)}"
   end
 end

The logs

🌍 --> GET https://dev.azure.com/tingle/dependabot/_apis/git/repositories/repro-366
🌍 <-- 200 https://dev.azure.com:443/tingle/dependabot/_apis/git/repositories/repro-366
🌍 --> GET https://dev.azure.com/tingle/dependabot/_apis/git/repositories/repro-366/stats/branches?name=main
🌍 <-- 200 https://dev.azure.com:443/tingle/dependabot/_apis/git/repositories/repro-366/stats/branches?name=main
🌍 --> GET https://dev.azure.com/tingle/dependabot/_apis/git/repositories/repro-366/items?path=.github/dependabot.yml&versionDescriptor.versionType=commit&versionDescriptor.version=484e0dccc2d7b6a3bbb886a01c1da23544de8c36
🌍 <-- 200 https://dev.azure.com:443/tingle/dependabot/_apis/git/repositories/repro-366/items?path=.github/dependabot.yml&versionDescriptor.versionType=commit&versionDescriptor.version=484e0dccc2d7b6a3bbb886a01c1da23544de8c36

Related to dependabot/dependabot-core#6459

I also fixed indentation for additions in #803

[geemus]

Thanks, I think that all makes sense. Just to make sure I'm getting it though, is it accurate to say that the logs you show here are the pre-patch behavior? ie I would think if I understand the change correctly that after the patch the request/response values should actually match up (with either both or neither containing the port, depending on the omit_default_port setting). Is that accurate?

[mburumaxwell]

Thanks, I think that all makes sense. Just to make sure I'm getting it though, is it accurate to say that the logs you show here are the pre-patch behavior? ie I would think if I understand the change correctly that after the patch the request/response values should actually match up (with either both or neither containing the port, depending on the omit_default_port setting). Is that accurate?

Yes this is correct.

[geemus]

Looks good, thanks!

[geemus]

@mburumaxwell did you need a release with this change?

[mburumaxwell]

@geemus it would be great to get a release. A patch maybe?

[geemus]

Released in v0.97.2.

@mburumaxwell Random thought, I wonder if we might eventually want to move toward something like a request_params key for the response data and group everything there, instead of having it mix request/response related stuff at the top level. This might also allow us to future-proof by pushing everything into that from request, instead of needing to cherry pick. That said, we now have released this and have things that might be depending on it, so it would be a breaking change (and therefore probably need some double writing and backwards compatibility stuff to transition). Does that make sense? What do you think? No immediate action needed here, just thinking aloud and hoping to get some feedback on whether we might want to head that way in the future.

[mburumaxwell]

Released in v0.97.2.

@mburumaxwell Random thought, I wonder if we might eventually want to move toward something like a request_params key for the response data and group everything there, instead of having it mix request/response related stuff at the top level. This might also allow us to future-proof by pushing everything into that from request, instead of needing to cherry pick. That said, we now have released this and have things that might be depending on it, so it would be a breaking change (and therefore probably need some double writing and backwards compatibility stuff to transition). Does that make sense? What do you think? No immediate action needed here, just thinking aloud and hoping to get some feedback on whether we might want to head that way in the future.

That would actually make things a lot easier because the information required for logging/instrumentation or other work would always be available. It makes a lot of sense.

[geemus]

@mburumaxwell - thanks for confirming. It seemed like it could in the long term make things much simpler and would allow any other changes to request to "just work". The in between parts seem like they could be a bit trickier though. I'll have to think a bit more on the specifics, but it's good to have a general idea of direction at least.

[mburumaxwell]

I have a random, possible not so-good, idea:

1. Migrate to a request_params key in the response data.
2. Add middleware (or configuration) that would add all the details in the request_params into the current params. Adding this middleware or enabling the configuration would result in the current behaviour.

Problem is it would break the functionality but maybe the reverse would work i.e. opt-in the new behaviour.

Also, thinking out-loud.

[geemus]

@mburumaxwell - I created a new issue with more details of this idea #811 so that we can discuss (and hopefully not forget) as needed. No action for now, just wanted to keep you in the loop.