tools/perf/util/metricgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Rather than place metrics without a metric group in "No_group" place
them in a a metric group that is their name. Still allow such metrics
to be selected if "No_group" is passed, this change just impacts perf
list.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/metricgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 79ef6095ab28..6ec083af14a1 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
const char *g;
char *omg, *mg;
- mg = strdup(pm->metric_group ?: "No_group");
+ mg = strdup(pm->metric_group ?: pm->metric_name);
if (!mg)
return -ENOMEM;
omg = mg;
@@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
if (strlen(g))
me = mep_lookup(groups, g, pm->metric_name);
else
- me = mep_lookup(groups, "No_group", pm->metric_name);
+ me = mep_lookup(groups, pm->metric_name, pm->metric_name);
if (me) {
me->metric_desc = pm->desc;
--
2.44.0.478.gd926399ef9-goog
On 2024-04-03 12:46 p.m., Ian Rogers wrote:
> Rather than place metrics without a metric group in "No_group" place
> them in a a metric group that is their name. Still allow such metrics
> to be selected if "No_group" is passed, this change just impacts perf
> list.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Thanks,
Kan
> ---
> tools/perf/util/metricgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 79ef6095ab28..6ec083af14a1 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> const char *g;
> char *omg, *mg;
>
> - mg = strdup(pm->metric_group ?: "No_group");
> + mg = strdup(pm->metric_group ?: pm->metric_name);
> if (!mg)
> return -ENOMEM;
> omg = mg;
> @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> if (strlen(g))
> me = mep_lookup(groups, g, pm->metric_name);
> else
> - me = mep_lookup(groups, "No_group", pm->metric_name);
> + me = mep_lookup(groups, pm->metric_name, pm->metric_name);
>
> if (me) {
> me->metric_desc = pm->desc;
On Fri, Apr 05, 2024 at 10:45:59AM -0400, Liang, Kan wrote: > > > On 2024-04-03 12:46 p.m., Ian Rogers wrote: > > Rather than place metrics without a metric group in "No_group" place > > them in a a metric group that is their name. Still allow such metrics > > to be selected if "No_group" is passed, this change just impacts perf > > list. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > Reviewed-by: Kan Liang <kan.liang@linux.intel.com> Thanks, applied to perf-tools-next, - Arnaldo
On Wed, Apr 03, 2024 at 09:46:36AM -0700, Ian Rogers wrote: > Rather than place metrics without a metric group in "No_group" place > them in a a metric group that is their name. Still allow such metrics > to be selected if "No_group" is passed, this change just impacts perf > list. But what's the point of it? It will just make perf list more verbose, but I don't see any advantage. -Andi
On Wed, Apr 3, 2024 at 11:44 AM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Wed, Apr 03, 2024 at 09:46:36AM -0700, Ian Rogers wrote:
> > Rather than place metrics without a metric group in "No_group" place
> > them in a a metric group that is their name. Still allow such metrics
> > to be selected if "No_group" is passed, this change just impacts perf
> > list.
>
> But what's the point of it? It will just make perf list more verbose,
> but I don't see any advantage.
So it is possible to list all metrics, that's not changed here. The
thing I'm looking to change is that when a metric is standalone it
appears in "perf list metricgroups". The reason is that a metric group
can gather a bunch of related metrics, say some form of read, write
and total bandwidth, whereas something like an idle metric
("d_ratio(max(msr@tsc@ - msr@mperf@, 0), msr@tsc@)") that could get
placed in No_group is more useful if it appears in a metric group of
"idle". I'd put forward that nobody ever wants to run "idle" as part
of "No_group" whereas being able to see it as a thing in metricgroups
is useful. I want to be able to run "perf list metricgroups" and get
groups of 1 or more metrics that someone might want to pass to "perf
stat -M", currently this just shows when there is a group of more than
1 metric as there is no practice of putting a metric like "idle" into
a metric group called "idle". We could update all metrics to make it
so that when they don't have a metric group we add them to one with
their name. We could do this in jevents.py. Those changes would make
the No_group logic redundant, so we should remove it. Just updating
the No_group logic in the perf command seemed like the minimal
approach.
Thanks,
Ian
> -Andi
On 2024-04-03 12:46 p.m., Ian Rogers wrote:
> Rather than place metrics without a metric group in "No_group" place
> them in a a metric group that is their name. Still allow such metrics
> to be selected if "No_group" is passed, this change just impacts perf
> list.
So it looks like the "No_group" is not completely removed.
They are just not seen in the perf list, but users can still use it via
perf stat -M No_group, right?
If so, why we want to remove it from perf list? Where can the end user
know which metrics are included in the No_group?
If the No_group is useless, why not completely remove it?
Thanks,
Kan
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/metricgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 79ef6095ab28..6ec083af14a1 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> const char *g;
> char *omg, *mg;
>
> - mg = strdup(pm->metric_group ?: "No_group");
> + mg = strdup(pm->metric_group ?: pm->metric_name);
> if (!mg)
> return -ENOMEM;
> omg = mg;
> @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> if (strlen(g))
> me = mep_lookup(groups, g, pm->metric_name);
> else
> - me = mep_lookup(groups, "No_group", pm->metric_name);
> + me = mep_lookup(groups, pm->metric_name, pm->metric_name);
>
> if (me) {
> me->metric_desc = pm->desc;
On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-03 12:46 p.m., Ian Rogers wrote:
> > Rather than place metrics without a metric group in "No_group" place
> > them in a a metric group that is their name. Still allow such metrics
> > to be selected if "No_group" is passed, this change just impacts perf
> > list.
>
> So it looks like the "No_group" is not completely removed.
> They are just not seen in the perf list, but users can still use it via
> perf stat -M No_group, right?
>
> If so, why we want to remove it from perf list? Where can the end user
> know which metrics are included in the No_group?
>
> If the No_group is useless, why not completely remove it?
Agreed. For command line argument deprecation we usually keep the
option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to
follow that pattern albeit that a metric group isn't a command line
option it's an option to an option.
Thanks,
Ian
> Thanks,
> Kan
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/metricgroup.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 79ef6095ab28..6ec083af14a1 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> > const char *g;
> > char *omg, *mg;
> >
> > - mg = strdup(pm->metric_group ?: "No_group");
> > + mg = strdup(pm->metric_group ?: pm->metric_name);
> > if (!mg)
> > return -ENOMEM;
> > omg = mg;
> > @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> > if (strlen(g))
> > me = mep_lookup(groups, g, pm->metric_name);
> > else
> > - me = mep_lookup(groups, "No_group", pm->metric_name);
> > + me = mep_lookup(groups, pm->metric_name, pm->metric_name);
> >
> > if (me) {
> > me->metric_desc = pm->desc;
On 2024-04-03 2:31 p.m., Ian Rogers wrote:
> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2024-04-03 12:46 p.m., Ian Rogers wrote:
>>> Rather than place metrics without a metric group in "No_group" place
>>> them in a a metric group that is their name. Still allow such metrics
>>> to be selected if "No_group" is passed, this change just impacts perf
>>> list.
>>
>> So it looks like the "No_group" is not completely removed.
>> They are just not seen in the perf list, but users can still use it via
>> perf stat -M No_group, right?
>>
>> If so, why we want to remove it from perf list? Where can the end user
>> know which metrics are included in the No_group?
>>
>> If the No_group is useless, why not completely remove it?
>
> Agreed. For command line argument deprecation we usually keep the
> option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to
> follow that pattern albeit that a metric group isn't a command line
> option it's an option to an option.
>
Perf list has a deprecated option to show the deprecated events.
The "No_group" should be a deprecated metrics group.
If so, to follow the same pattern, I think perf list should still
display the "No_group" with the --deprecated option at least.
Thanks,
Kan
> Thanks,
> Ian
>
>> Thanks,
>> Kan
>>
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>> tools/perf/util/metricgroup.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>>> index 79ef6095ab28..6ec083af14a1 100644
>>> --- a/tools/perf/util/metricgroup.c
>>> +++ b/tools/perf/util/metricgroup.c
>>> @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
>>> const char *g;
>>> char *omg, *mg;
>>>
>>> - mg = strdup(pm->metric_group ?: "No_group");
>>> + mg = strdup(pm->metric_group ?: pm->metric_name);
>>> if (!mg)
>>> return -ENOMEM;
>>> omg = mg;
>>> @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
>>> if (strlen(g))
>>> me = mep_lookup(groups, g, pm->metric_name);
>>> else
>>> - me = mep_lookup(groups, "No_group", pm->metric_name);
>>> + me = mep_lookup(groups, pm->metric_name, pm->metric_name);
>>>
>>> if (me) {
>>> me->metric_desc = pm->desc;
On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-03 2:31 p.m., Ian Rogers wrote:
> > On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >>
> >>
> >> On 2024-04-03 12:46 p.m., Ian Rogers wrote:
> >>> Rather than place metrics without a metric group in "No_group" place
> >>> them in a a metric group that is their name. Still allow such metrics
> >>> to be selected if "No_group" is passed, this change just impacts perf
> >>> list.
> >>
> >> So it looks like the "No_group" is not completely removed.
> >> They are just not seen in the perf list, but users can still use it via
> >> perf stat -M No_group, right?
> >>
> >> If so, why we want to remove it from perf list? Where can the end user
> >> know which metrics are included in the No_group?
> >>
> >> If the No_group is useless, why not completely remove it?
> >
> > Agreed. For command line argument deprecation we usually keep the
> > option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to
> > follow that pattern albeit that a metric group isn't a command line
> > option it's an option to an option.
> >
>
> Perf list has a deprecated option to show the deprecated events.
> The "No_group" should be a deprecated metrics group.
>
> If so, to follow the same pattern, I think perf list should still
> display the "No_group" with the --deprecated option at least.
Such metrics would be shown twice, once under No_group and once under
a metric group of their name. With deprecated events this isn't the
case, you can only see them with --deprecated. Given we can see the
metric without the No_group grouping, what is being added by having a
No_group grouping? It feels entirely redundant and something we don't
need to advertise.
Thanks,
Ian
> Thanks,
> Kan
>
> > Thanks,
> > Ian
> >
> >> Thanks,
> >> Kan
> >>
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>> tools/perf/util/metricgroup.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> >>> index 79ef6095ab28..6ec083af14a1 100644
> >>> --- a/tools/perf/util/metricgroup.c
> >>> +++ b/tools/perf/util/metricgroup.c
> >>> @@ -455,7 +455,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> >>> const char *g;
> >>> char *omg, *mg;
> >>>
> >>> - mg = strdup(pm->metric_group ?: "No_group");
> >>> + mg = strdup(pm->metric_group ?: pm->metric_name);
> >>> if (!mg)
> >>> return -ENOMEM;
> >>> omg = mg;
> >>> @@ -466,7 +466,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
> >>> if (strlen(g))
> >>> me = mep_lookup(groups, g, pm->metric_name);
> >>> else
> >>> - me = mep_lookup(groups, "No_group", pm->metric_name);
> >>> + me = mep_lookup(groups, pm->metric_name, pm->metric_name);
> >>>
> >>> if (me) {
> >>> me->metric_desc = pm->desc;
On 2024-04-03 4:26 p.m., Ian Rogers wrote: > On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >> >> >> >> On 2024-04-03 2:31 p.m., Ian Rogers wrote: >>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >>>> >>>> >>>> >>>> On 2024-04-03 12:46 p.m., Ian Rogers wrote: >>>>> Rather than place metrics without a metric group in "No_group" place >>>>> them in a a metric group that is their name. Still allow such metrics >>>>> to be selected if "No_group" is passed, this change just impacts perf >>>>> list. >>>> >>>> So it looks like the "No_group" is not completely removed. >>>> They are just not seen in the perf list, but users can still use it via >>>> perf stat -M No_group, right? >>>> >>>> If so, why we want to remove it from perf list? Where can the end user >>>> know which metrics are included in the No_group? >>>> >>>> If the No_group is useless, why not completely remove it? >>> >>> Agreed. For command line argument deprecation we usually keep the >>> option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to >>> follow that pattern albeit that a metric group isn't a command line >>> option it's an option to an option. >>> >> >> Perf list has a deprecated option to show the deprecated events. >> The "No_group" should be a deprecated metrics group. >> >> If so, to follow the same pattern, I think perf list should still >> display the "No_group" with the --deprecated option at least. > > Such metrics would be shown twice, once under No_group and once under > a metric group of their name. You mean with the --deprecated option? Yes, that's because the old/deprecated metrics group (No_group) is not complete removed. So both the new name and old/deprecated name are shown with the --deprecated option. The metrics which belong to both groups will be shown twice. Without the --deprecated option, only the new group and its members are shown. > With deprecated events this isn't the > case, you can only see them with --deprecated. Given we can see the > metric without the No_group grouping, what is being added by having a > No_group grouping? It feels entirely redundant and something we don't > need to advertise. I just want to have a generic pattern for deprecating a metrics/metrics group that everybody can follow. I treat the "No_group" as a normal metrics group name. So this patch is to introduce a new name, and hide the old name. Both new and old names can still be used. If it's for a deprecated event, the expectation is to only see the new name by default, and see both new name and old name with the --deprecated option. Now, if it's a generic deprecated metrics group, what's the expected behavior? I prefer to follow the same pattern as a deprecated event. If we do so, yes, there will be some redundancy with the --deprecated option, since some members may belong to both old and new groups. But I don't think it's an issue. It's normal that metrics belong to different groups. Thanks, Kan
On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote: > > > > On 2024-04-03 4:26 p.m., Ian Rogers wrote: > > On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote: > >> > >> > >> > >> On 2024-04-03 2:31 p.m., Ian Rogers wrote: > >>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2024-04-03 12:46 p.m., Ian Rogers wrote: > >>>>> Rather than place metrics without a metric group in "No_group" place > >>>>> them in a a metric group that is their name. Still allow such metrics > >>>>> to be selected if "No_group" is passed, this change just impacts perf > >>>>> list. > >>>> > >>>> So it looks like the "No_group" is not completely removed. > >>>> They are just not seen in the perf list, but users can still use it via > >>>> perf stat -M No_group, right? > >>>> > >>>> If so, why we want to remove it from perf list? Where can the end user > >>>> know which metrics are included in the No_group? > >>>> > >>>> If the No_group is useless, why not completely remove it? > >>> > >>> Agreed. For command line argument deprecation we usually keep the > >>> option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to > >>> follow that pattern albeit that a metric group isn't a command line > >>> option it's an option to an option. > >>> > >> > >> Perf list has a deprecated option to show the deprecated events. > >> The "No_group" should be a deprecated metrics group. > >> > >> If so, to follow the same pattern, I think perf list should still > >> display the "No_group" with the --deprecated option at least. > > > > Such metrics would be shown twice, once under No_group and once under > > a metric group of their name. > You mean with the --deprecated option? > Yes, that's because the old/deprecated metrics group (No_group) is not > complete removed. So both the new name and old/deprecated name are shown > with the --deprecated option. The metrics which belong to both groups > will be shown twice. > > Without the --deprecated option, only the new group and its members are > shown. > > > With deprecated events this isn't the > > case, you can only see them with --deprecated. Given we can see the > > metric without the No_group grouping, what is being added by having a > > No_group grouping? It feels entirely redundant and something we don't > > need to advertise. > > I just want to have a generic pattern for deprecating a metrics/metrics > group that everybody can follow. Currently there is no concept of a metric group in the json except for descriptions. Metrics and events share the same encoding, and the deprecated flag belongs to the event. > I treat the "No_group" as a normal metrics group name. So this patch is > to introduce a new name, and hide the old name. Both new and old names > can still be used. Why are you using No_group? I stand firm that it has no real use. > If it's for a deprecated event, the expectation is to only see the new > name by default, and see both new name and old name with the > --deprecated option. > > Now, if it's a generic deprecated metrics group, what's the expected > behavior? I prefer to follow the same pattern as a deprecated event. > If we do so, yes, there will be some redundancy with the --deprecated > option, since some members may belong to both old and new groups. > But I don't think it's an issue. It's normal that metrics belong to > different groups. What you are requesting here isn't something that is unreasonable, it is just something unconnected with this change and requires a reorganization of the json to facilitate. As such I consider it to be something for follow up work. I think if we're going to restructure metric groups it would be nice to add a more tree-like structure which could be used to visualize metrics better. For example here: https://lore.kernel.org/lkml/20240314055919.1979781-11-irogers@google.com/ the metrics could be shown under a tree like: ldst - ldst_total - ldst_total_loads - ldst_prcnt - ldst_prcnt_loads - ldst_prcnt_stores - ldst_ret_lds - ldst_ret_lds_1 - ldst_ret_lds_2 - ldst_ret_lds_3 - ldst_ret_sts - ldst_ret_sts_1 - ldst_ret_sts_2 - ldst_ret_sts_3 - ldst_ld_hit_swpf - ldst_atomic_lds but again it is follow up work to do this. In this change I just wanted a way to list all sensibly grouped metrics or metrics in a group just on their own which doesn't require some kind of analysis of metric groups. No_group has no use so let's just get rid of it. Thanks, Ian > Thanks, > Kan
On 2024-04-04 9:16 p.m., Ian Rogers wrote: > On Thu, Apr 4, 2024 at 1:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote: >> >> >> >> On 2024-04-03 4:26 p.m., Ian Rogers wrote: >>> On Wed, Apr 3, 2024 at 11:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >>>> >>>> >>>> >>>> On 2024-04-03 2:31 p.m., Ian Rogers wrote: >>>>> On Wed, Apr 3, 2024 at 10:59 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2024-04-03 12:46 p.m., Ian Rogers wrote: >>>>>>> Rather than place metrics without a metric group in "No_group" place >>>>>>> them in a a metric group that is their name. Still allow such metrics >>>>>>> to be selected if "No_group" is passed, this change just impacts perf >>>>>>> list. >>>>>> >>>>>> So it looks like the "No_group" is not completely removed. >>>>>> They are just not seen in the perf list, but users can still use it via >>>>>> perf stat -M No_group, right? >>>>>> >>>>>> If so, why we want to remove it from perf list? Where can the end user >>>>>> know which metrics are included in the No_group? >>>>>> >>>>>> If the No_group is useless, why not completely remove it? >>>>> >>>>> Agreed. For command line argument deprecation we usually keep the >>>>> option but hide it from help with PARSE_OPT_HIDDEN, so I was trying to >>>>> follow that pattern albeit that a metric group isn't a command line >>>>> option it's an option to an option. >>>>> >>>> >>>> Perf list has a deprecated option to show the deprecated events. >>>> The "No_group" should be a deprecated metrics group. >>>> >>>> If so, to follow the same pattern, I think perf list should still >>>> display the "No_group" with the --deprecated option at least. >>> >>> Such metrics would be shown twice, once under No_group and once under >>> a metric group of their name. >> You mean with the --deprecated option? >> Yes, that's because the old/deprecated metrics group (No_group) is not >> complete removed. So both the new name and old/deprecated name are shown >> with the --deprecated option. The metrics which belong to both groups >> will be shown twice. >> >> Without the --deprecated option, only the new group and its members are >> shown. >> >>> With deprecated events this isn't the >>> case, you can only see them with --deprecated. Given we can see the >>> metric without the No_group grouping, what is being added by having a >>> No_group grouping? It feels entirely redundant and something we don't >>> need to advertise. >> >> I just want to have a generic pattern for deprecating a metrics/metrics >> group that everybody can follow. > > Currently there is no concept of a metric group in the json except for > descriptions. Metrics and events share the same encoding, and the > deprecated flag belongs to the event. > >> I treat the "No_group" as a normal metrics group name. So this patch is >> to introduce a new name, and hide the old name. Both new and old names >> can still be used. > > Why are you using No_group? I stand firm that it has no real use. > >> If it's for a deprecated event, the expectation is to only see the new >> name by default, and see both new name and old name with the >> --deprecated option. >> >> Now, if it's a generic deprecated metrics group, what's the expected >> behavior? I prefer to follow the same pattern as a deprecated event. >> If we do so, yes, there will be some redundancy with the --deprecated >> option, since some members may belong to both old and new groups. >> But I don't think it's an issue. It's normal that metrics belong to >> different groups. > > What you are requesting here isn't something that is unreasonable, it > is just something unconnected with this change and requires a > reorganization of the json to facilitate. As such I consider it to be > something for follow up work. > > I think if we're going to restructure metric groups it would be nice > to add a more tree-like structure which could be used to visualize > metrics better. For example here: > https://lore.kernel.org/lkml/20240314055919.1979781-11-irogers@google.com/ > the metrics could be shown under a tree like: > ldst > - ldst_total > - ldst_total_loads > - ldst_prcnt > - ldst_prcnt_loads > - ldst_prcnt_stores > - ldst_ret_lds > - ldst_ret_lds_1 > - ldst_ret_lds_2 > - ldst_ret_lds_3 > - ldst_ret_sts > - ldst_ret_sts_1 > - ldst_ret_sts_2 > - ldst_ret_sts_3 > - ldst_ld_hit_swpf > - ldst_atomic_lds > Yes, a tree-like output looks much better. > but again it is follow up work to do this. In this change I just > wanted a way to list all sensibly grouped metrics or metrics in a > group just on their own which doesn't require some kind of analysis of > metric groups. No_group has no use so let's just get rid of it. > I agree that there should be no one to use the No_group. Just hide it should be fine. Maybe we can have further discussion when someday we try to deprecate a meaningful metrics/metrics group. Thanks, Kan
© 2016 - 2026 Red Hat, Inc.