WeTransfer / format_parser

file metadata parsing, done cheap
https://rubygems.org/gems/format_parser
Other
62 stars 18 forks source link

[PREV-144] Loosen/Drop Dependencies, Drop EOL Support and Rename Measurometer Metrics #215

Closed Kevin-McGonigle closed 2 years ago

Kevin-McGonigle commented 2 years ago
enricoribelli commented 2 years ago

Nice big cleanup!! 🤩

enricoribelli commented 2 years ago

question just to double confirm: deprecation error is not flooding anymore in DD, right?

Kevin-McGonigle commented 2 years ago

question just to double confirm: deprecation error is not flooding anymore in DD, right?

I'll deploy to dev and staging today to confirm.

enricoribelli commented 2 years ago

question just to double confirm: deprecation error is not flooding anymore in DD, right?

I'll deploy to dev and staging today to confirm.

cool thanks! give me thumbs up after that and I'll ✅ :)

Kevin-McGonigle commented 2 years ago

I have two more remarks, aside from the ones in the review:

  1. It's great we got rid of Faraday, but it would be good to make sure this doesn't bring any functional/performance penalty. I can imagine there was a good reason Faraday was used, so we should make our due diligence while removing it
  2. I understand why we've renamed most metrics, however I feel this will break too many things for too many projects for not that much of an advantage. Are we sure we want to go down this path?

(GitHub, why can't I just reply to this comment in a thread? FFS)

  1. Sounds like a good idea! I was gonna do this by letting it loose in staging for a while and keeping an eye on DataDog, but I don't love that as an approach... do you know what the best way to go about that?

  2. The renamed metrics came in as part of #217, which you approved and suggested be included as part of this major release. Have you changed your thoughts on it?

@linkyndy

linkyndy commented 2 years ago

Yes, would be great if they implemented that feature.

  1. That's a good start. We can draft a 2.0.0.pre release of this gem and test drive that in the wild. We can load test locally, but ultimately it should face the real deal once we are confident enough about the progress.
  2. I might have, yes. Thinking more about it, I cannot convince myself there is an actual benefit from causing gem users this amount of pain 😅
Kevin-McGonigle commented 2 years ago

@linkyndy

  1. Cool, I've switched the version to a prerelease.

  2. @grdw, could you weigh in on the motivation for renaming the metrics and whether or not it's worth the external headaches it might cause?

grdw commented 2 years ago

could you weigh in on the motivation for renaming the metrics and whether or not it's worth the external headaches it might cause?

It's to not overflow metric systems with a counter per file type, but rather use tags for these kinds of situations. Appsignal/Datadog /etc. all support this feature. And it's to keep the counters consistent (so, keep it all in snake case).

[..] and whether or not it's worth the external headaches it might cause?

Do we know if any other companies/people besides WeTransfer use format parser?

linkyndy commented 2 years ago

As far as I know, there are others using this gem, yes.

grdw commented 2 years ago

I understand why we've renamed most metrics, however I feel this will break too many things for too many projects for not that much of an advantage. Are we sure we want to go down this path?

Again, if we're not sure how many projects use the actual gem, and moreover use the gem and the metrics, this is not much of a statement 💭. As long as we described in the CHANGELOG that we changed it, then we should be good as far as I'm concerned.

We can also keep the metrics as is; with the funny snake-camel-case, fine with me as well 🤷. But then what? How will we ever change it in the future?

linkyndy commented 2 years ago

Fair point. Feels like we've reached a consensus on this one. Let's 🚀 then!