Missing trailing period.

I would add a short sentence about this behavior in the comment.

Why not inline? testutil.Ok(t, suite.Run())

Maybe minor and not a blocker but in this case it is quite confusing to add two labels and base on the fact that one of those is empty. Also we unnecessarily add a couple of adds and dels to the builder just to avoid one if. Sound unnecessary to me. I would leave as this maybe?

lb.Set(l.Name, l.Value)
lv := lset.Get(l.Name)
if lv != "" {	
    lb.Set(model.ExportedLabelPrefix+l.Name, lv)	
}

Essentially it quite hard to read this code, maybe comment would help? as well?

Even better, can we use mutateReportSampleLabels here? We duplicate code essentially.

base on the fact that one of those is empty

That's not what this logic is doing, both values can be non-empty.

can we use mutateReportSampleLabels here?

I think that'd be less clear, there's different business logic for each.

I think the general approach is fine, but it is a bit arcane. How about the following for the body of the for loop:

// existingValue will be empty if l.Name doesn't exist.
existingValue := lset.Get(l.Name)
// Because setting a label with an empty value is a no-op,
// this will only create the prefixed label if necessary.
lb.Set(model.ExportedLabelPrefix+l.Name, existingValue)
// It is now safe to set the target label.
lb.Set(l.Name, l.Value)

In different news, while thinking through the code, I realised we have edge cases that are underspecified in our docs:

1. What happens if the target labels contain both foo="a" and exported_foo="b" and then a scrape contains its own label foo="c"? End result could be:
   1. `foo="a",exported_foo="b"
   2. `foo="a",exported_foo="c"
   3. `foo="a",exported_foo="b",exported_exported_foo="c"
2. What happens if a scrape contains both foo="a" and exported_foo="b" and then there is a target label foo="c"? End result could be:
   1. `foo="c",exported_foo="a"
   2. `foo="c",exported_foo="b"
   3. `foo="c",exported_foo="a",exported_exported_foo="b"

In case 2, the code will do i. But as said, that's not precisely specified in the docs. One might also expect iii.

Case 1 is more interesting, as the behavior depends on the order the target labels show up in the iteration. It could be i. or iii. That's in any case an undesired behavior.

For double fun, case 1 and case 2 could be combined (both the scrape and the target labels contain both a foo and an exported_foo label).

💥

Yes, this is unspecified. Noone has complained about it after many years, so I haven't worried about it thus far.

Don't do this ;)

We should at least document that the behavior is undefined if labels with the exported_ prefix are part of the game.

I think, however, that we can do better. Especially the more or less random choice of behavior in case 2 worries me.

Made your suggested code change.

The proper way would be iii, and that any exported_ scraped label would always be prefixed. It's technically a breaking change.

Yeah, just by reading the docs, I'd expect iii., although it's not very clearly expressed. We could still claim we are just bringing the implementation in line with the (vaguely) specified behavior. Thus, bug fix, not breaking change. https://xkcd.com/1172/

I'll file a separate issue to keep track.

can we change lset to extLset to be clear? Was quite confused until I figured out those are external labels.

It's not the external labels though, they're not involved in scraping. These are the sample labels.

I think the general approach is fine, but it is a bit arcane. How about the following for the body of the for loop:

// existingValue will be empty if l.Name doesn't exist.
existingValue := lset.Get(l.Name)
// Because setting a label with an empty value is a no-op,
// this will only create the prefixed label if necessary.
lb.Set(model.ExportedLabelPrefix+l.Name, existingValue)
// It is now safe to set the target label.
lb.Set(l.Name, l.Value)

In different news, while thinking through the code, I realised we have edge cases that are underspecified in our docs:

1. What happens if the target labels contain both foo="a" and exported_foo="b" and then a scrape contains its own label foo="c"? End result could be:
   1. `foo="a",exported_foo="b"
   2. `foo="a",exported_foo="c"
   3. `foo="a",exported_foo="b",exported_exported_foo="c"
2. What happens if a scrape contains both foo="a" and exported_foo="b" and then there is a target label foo="c"? End result could be:
   1. `foo="c",exported_foo="a"
   2. `foo="c",exported_foo="b"
   3. `foo="c",exported_foo="a",exported_exported_foo="b"

In case 2, the code will do i. But as said, that's not precisely specified in the docs. One might also expect iii.

Case 1 is more interesting, as the behavior depends on the order the target labels show up in the iteration. It could be i. or iii. That's in any case an undesired behavior.

For double fun, case 1 and case 2 could be combined (both the scrape and the target labels contain both a foo and an exported_foo label).

💥