I still think this function would be more accurately called something like metricNameExists(...) bool.

Also, I didn't catch before - what was the main justification for keeping this validation? I'm still 50/50 on whether its needed at all. While it would imply an error to have two metrics with the same name, I don't think there's any other code to validate that there aren't already duplicates in this list (which if there were, could result in unexpected behaviour in your code), and I'm not sure its worth paying the cost of allocating this extra map, having more complex code, etc.

It's likely an error will be thrown at export time if this situation occurs anyway.

If you do agree with me about removing this validation, you can remove a huge amount of the code in the core functions of this file, i.e. the transform function can be simplfied to:

for _, metric := range data.Metrics {
    transform := nameToTransformMapping[metric.MetricDescriptor.Name]

    if transform.Action == Insert {
        metric = mtp.createCopy(metric)
        mds[i].Metrics = append(metricPtrs, metricCopy)
    }

    mtp.update(metricCopy, transform)
}

The nameToTransformMapping can be made once on initialization

So this validNewName and other validation functions came out of discussions with Dave and Quentin. Basically, when these unexpected behaviors occur, not only does it appear in the output data, but there will be an error message logged, so that the errors can be more viewable to the users

Also regarding simplifying the code this way, since we want the transformations to perform in the order from the list. I would think we need to iterate through the transforms instead of the metric, which then involves managing the metrics mapping as we go though transformations.

Yea okay that's true. I was hoping to find a solution that would avoid having to allocate an extra map on each call to ConsumeMetrics, but if we want to guarantee the order of operations that is probably not possible.

I am also slightly concerned that if we ever extend this processor to support filtering beyond just "name", then it won't make sense to have a simple map anymore, but we can figure out how to handle that later

This creates a bit of weird behaviour where the order of the actions may matter in terms of determining whether an error will occur. This is another reason to just remove this logic entirely imo (see comment on L182)

I thought the order of transformations here should matter because when I discussed with Dave and Quentin, the transforms list should be an ordered action list that will perform the transforms one by one to the metrics. Do you think that this shouldn't be the case?

Nit: you can put mds[i] into a variable so you don't need to dereference it on each iteration

I actually tried to use data directly, which is mds[i] from the for loop, but this way, the original copy of the metric is not updated correctly since mds is a list of non-pointers.

Oh interesting. I guess you could set data := &mds[i] instead

This code is relatively minimal and straightforward, but even still, similar to above, I think I'm a fan of doing little / no validation here and allowing this processor to be lean and fast. If the user screws up, they will export bad data, or more likely get an error at export time.

Happy to be convinced otherwise on this one though.

This would be for the same reasoning as the other validations. (This one is to guard against renaming a label to a name that already exists because this should be done through an aggregation) These are all to keep the boundaries of the expected behaviors, and once the operations step out of bounds, the processor is able to notice. However, after thinking about the single responsibility of the processor, I do see how these validations should be removed. Since the processors should only worry about transforming the data specified by the user even the user's specification might imply a possible error. I also don't see any other processors that validate data processing, so from a convention stand point, I also think removing the validations might be reasonable. However, this also means that users may have data transformed in ways that might result in bad output metric data. If this is ok for processors, then with your approval, I would love to remove the validations because this also makes the code a lot more straightforward.

My opinion is lets start with not having it and keep the code very simple. We can add it in later if people want it. But I'm okay with leaving it there if you feel strongly about it

No problem! The current plan is removing the validations for the purpose of starting simple, but I will keep this code saved in another branch. If it is needed later, we will put this back in. If we later decide to add this back in, I will need to modify the logging process as well. Therefore, before I do any additional work on that, I want to make sure the work is needed.