Hi Ryan, If I look at the changelog for the simpleclient 0.10 [1], they've switched their data model. So if you upgrade to the later version, the data model for existing Flink Prometheus users would be broken IIRC. That's why I think option 1 is more clean: it provides the option to the user to choose which package they want to use. Either the new one, with a new data model, or the current one, with the existing data model.
Best regards, Martijn [1] https://github.com/prometheus/client_java/releases/tag/parent-0.10.0 On Fri, Jun 30, 2023 at 4:23 PM Ryan van Huuksloot <ryan.vanhuuksl...@shopify.com.invalid> wrote: > I'd have to check but the original plan is to upgrade the client but keep > the flink-metrics-prometheus implementation the same. This should keep the > metrics collection consistent even with the client upgrade - this would > need to be verified. > > But if that is the worry then we could create a new package to keep things > distinct. > > Thanks, > > Ryan van Huuksloot > Sr. Production Engineer | Streaming Platform > [image: Shopify] > <https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email> > > > On Fri, Jun 30, 2023 at 10:02 AM Martijn Visser <martijnvis...@apache.org> > wrote: > > > Hi Patrick, > > > > Yeah, but you would need the latest version of the client, which would > > break the implementation for the current, outdated one, wouldn't it? > > > > Best regards, > > > > Martijn > > > > On Fri, Jun 30, 2023 at 3:35 PM Ryan van Huuksloot > > <ryan.vanhuuksl...@shopify.com.invalid> wrote: > > > > > Hi Martijn, > > > > > > Option 2 and 3 would use a single client. It would just register the > > > metrics differently. > > > > > > Does that make sense? Does that change your perspective? > > > > > > Thanks, > > > > > > Ryan van Huuksloot > > > Sr. Production Engineer | Streaming Platform > > > [image: Shopify] > > > < > https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email > > > > > > > > > > > > On Fri, Jun 30, 2023 at 7:49 AM Martijn Visser < > martijnvis...@apache.org > > > > > > wrote: > > > > > > > Hi Ryan, > > > > > > > > I think option 2 and option 3 won't work, because there can be only > one > > > > version of the client. I don't think we should make a clean break on > > > > metrics in a minor version, but only in major. All in all, I think > > > option 1 > > > > would be the best. We could deprecate the existing one and remove it > > > > with Flink 2.0 imho. > > > > > > > > Best regards, > > > > > > > > Martijn > > > > > > > > On Thu, Jun 29, 2023 at 5:56 PM Ryan van Huuksloot > > > > <ryan.vanhuuksl...@shopify.com.invalid> wrote: > > > > > > > > > Hi Martijn, > > > > > > > > > > Our team shared the same concern. We've considered a few options: > > > > > > > > > > > > > > > *1. Add a new package such as `flink-metrics-prometheus-native` and > > > > > eventually deprecate the original.* > > > > > *Pros:* > > > > > - Supports backward compatibility. > > > > > *Cons:* > > > > > - Two packages to maintain in the interim. > > > > > - Not consistent with other metrics packages. > > > > > > > > > > *2. Maintain the same logic in flink-metrics-prometheus and write > new > > > > > natively typed metrics to a different metric name in Prometheus, in > > > > > addition to the original metric.* > > > > > > > > > > *Pros:* > > > > > - Supports backward compatibility. > > > > > *Cons:* > > > > > - Nearly doubles the metrics being captured by default. > > > > > - The naming convention will permanently differ when the original > > names > > > > are > > > > > deprecated. > > > > > - The original names will likely be deprecated at some point. > > > > > > > > > > *3. Maintain the same logic in flink-metrics-prometheus. However, > if > > > you > > > > > use a flink-conf option, natively typed metrics would be written to > > the > > > > > same names instead of the original metric types.* > > > > > > > > > > *Pros:* > > > > > - Supports backwards compatibility > > > > > - No double metrics > > > > > *Cons:* > > > > > - Increases the maintenance burden. > > > > > - Would require future migrations > > > > > > > > > > *4. Make a clean break and swap the types in > > flink-metrics-prometheus, > > > > > releasing it in 1.18 or 1.19 with a note.* > > > > > > > > > > *Pros:* > > > > > - Avoids duplicate metrics and packages. > > > > > - No future maintenance burden. > > > > > *Cons:* > > > > > -Introduces a breaking change. > > > > > - Metrics may silently fail in dashboards if the graphs do not > > support > > > > the > > > > > new data type (I would need to conduct more testing to determine > how > > > > often > > > > > this occurs). > > > > > > > > > > I lean towards option 4, and we would communicate the change > > internally > > > > as > > > > > part of a minor version upgrade. I'm open to other ideas and would > > > > welcome > > > > > further discussion on what the OSS community prefers. > > > > > > > > > > Thanks, > > > > > > > > > > Ryan van Huuksloot > > > > > Sr. Production Engineer | Streaming Platform > > > > > [image: Shopify] > > > > > < > > > > https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email > > > > > > > > > > > > > > > > > > > > On Thu, Jun 29, 2023 at 4:23 AM Martijn Visser < > > > martijnvis...@apache.org > > > > > > > > > > wrote: > > > > > > > > > > > Hi Ryan, > > > > > > > > > > > > I think there definitely is an interest in the > > > > > > flink-metrics-prometheus, but I do see some challenges as well. > > Given > > > > > > that the Prometheus simpleclient doesn't yet have a major > version, > > > > > > there are breaking changes happening in that. If we would update > > > this, > > > > > > it can/probably breaks the metrics for users, which is an > > undesirable > > > > > > situation. Any thoughts on how we could avoid that situation? > > > > > > > > > > > > Best regards, > > > > > > > > > > > > Martijn > > > > > > > > > > > > On Tue, Jun 20, 2023 at 3:53 PM Ryan van Huuksloot > > > > > > <ryan.vanhuuksl...@shopify.com.invalid> wrote: > > > > > > > > > > > > > > Following up, any interest in flink-metrics-prometheus? It is > > > quite a > > > > > > stale > > > > > > > package. I would be interested in contributing - time > permitting. > > > > > > > > > > > > > > Ryan van Huuksloot > > > > > > > Sr. Production Engineer | Streaming Platform > > > > > > > [image: Shopify] > > > > > > > < > > > > > > > > > https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 15, 2023 at 2:16 PM Ryan van Huuksloot < > > > > > > > ryan.vanhuuksl...@shopify.com> wrote: > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > Internally we use the flink-metrics-prometheus jar and we > > noticed > > > > > that > > > > > > the > > > > > > > > code is a little out of date. Primarily, there are new metric > > > types > > > > > in > > > > > > > > Prometheus that would allow for the exporter to write > Counters > > > and > > > > > > > > Histograms as Native metrics in prometheus (vs writing as > > > Gauges). > > > > > > > > > > > > > > > > I noticed that there was a closed PR for the simpleclient: > > > > > > > > https://github.com/apache/flink/pull/21047 - which has what > is > > > > > > required > > > > > > > > for the native metrics but may cause other maintenance > tickets. > > > > > > > > > > > > > > > > Is there any appetite from the community to update this > > exporter? > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Ryan van Huuksloot > > > > > > > > Sr. Production Engineer | Streaming Platform > > > > > > > > [image: Shopify] > > > > > > > > < > > > > > > > > > > > > https://www.shopify.com/?utm_medium=salessignatures&utm_source=hs_email> > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
