pwmetrics icon indicating copy to clipboard operation
pwmetrics copied to clipboard

Feature request: ability to filter hiddenMetrics

Open MickeyKay opened this issue 7 years ago • 8 comments

At present it doesn't look like there's any way to filter/customize the metrics.hiddenMetrics array, which is used here to determine which metrics are output in the chart: https://github.com/paulirish/pwmetrics/blob/master/lib/index.ts#L214

Unless I'm mistaken, there is no way to customize the chart output with these hidden metrics, correct? If that is the case, I'd like to request the ability to do so. Seems like there are a few ways to do this, however if you want to summarize your preferred approach I'd be happy to take a pass at a PR if that's helpful. Thanks!

MickeyKay avatar May 11 '17 16:05 MickeyKay

In fact, the more I look into this the more it seems like it might be more extensible to switch over to using an includedMetrics, which would allow for a much more configurable top-level metrics object in a user's config.js file.

My use case is one in which we don't need to focus on expectations so much as basic metrics visualization (e.g. we just need to know the timings on specific metrics, not whether or not they meet a certain expectation). At present, pwmetrics is set up well for customizing output when expectations are specified, however I would like the ability to customize output for basic metrics visualization, without any expectations set.

I could imagine something like this being useful when no expectations are set, for example:

module.exports = {
  url: 'http://example.com/',
  flags: {
    expectations: false
  },
  metrics: {
    // specify which metrics to include, as well as related config
    ttfcp: {
      color: 'blue' // Just an example
    },
    ttfmp: {
      color: 'green'
    }
  },
  sheets: {
    ...
  },
  clientSecret: {
    ...
  }
}

MickeyKay avatar May 11 '17 16:05 MickeyKay

cc @paulirish @denar90. What do you guys think? This is somewhat similar to a feature we discussed a while back during the 2.0 release. Should we resume the conversation?

@MickeyKay how custom do you need metrics for your use? Simply have the option to include the hidden ones or something more than that?

pedro93 avatar May 11 '17 17:05 pedro93

@pedro93 at this point it's really just the ability to include/exclude metrics more granularly. That said, it seems like using the above, generic metrics object allows much more future extensibility, even if you only enable a list of metric names at this point.

Also, just curious, how did y'all decide to go the hiddenMetrics route, and how did you decide which metrics should/shouldn't be included by default?

Generally, btw, I think this issue and a few others I've filed all point to a broader theme of decoupling output configuration from expectations. I really really love the expectations functionality, however it'd be nice to have that just supplement the core output functionality, as opposed to defining it as is the case now.

MickeyKay avatar May 11 '17 18:05 MickeyKay

Just spun up https://github.com/paulirish/pwmetrics/pull/116 as a prototype for what this could look like. In an ideal world, we'd extract out a full metrics object in config I think, however this gets more complicated since we're importing the metrics object which has a lot more than just config.

MickeyKay avatar May 11 '17 19:05 MickeyKay

So, my thoughts about it. First of all, this looks breaking so it should go with v3. After LH major release.


Going with proposed solution we might face several problems:

  • what about sheets option? should it stick to metrics prop? if so, it makes complicated sheets. e.g I already have 10 rows and accidentally changed the list of metrics, then graph can be broken...
  • what about json/file output? Right now we show/save all metrics but display on chart only some of them.
  • more...

So, I dunno..

denar90 avatar May 12 '17 14:05 denar90

I would also like to add to what @denar90 said. A similar functionality to this has in the past been proposed and it included corner cases such as defining what the median run of out N runs is, see https://github.com/paulirish/pwmetrics/pull/71#issuecomment-285147886

pedro93 avatar May 12 '17 14:05 pedro93

Thanks for the feedback, I see. My thinking was that this would only affect charts for v1, since that is the current behavior of hiddenMetrics. An alternative, less sweeping change could be including a new chart top level config prop, however I realize this is getting a bit into the weeds now. Regardless, I would love to hear your thoughts on the simplest solution to allow customization of chart output. Right now there's no way to do this, correct?

Thanks for all the feedback!

MickeyKay avatar May 12 '17 17:05 MickeyKay

Sorry for the late reply @MickeyKay. To answer your question, imo opinion would be to allow the user to specify the metrics they want and having a default to fallback to if nothing is specified. See https://github.com/paulirish/pwmetrics/pull/71 for the discussion and implementation (at the time). As it stands currently, there is no way to this at the moment without changing the codebase.

pedro93 avatar Jun 22 '17 07:06 pedro93