[sqlqueryreceiver] - add optional static_attributes to metrics.

[Konig-Corey]

Description:

Adding a feature:

• Add ability to set static tags for metrics returned from queries

Testing:

• Added config test to validate config is loading a map of strings for tags.

Documentation:

• Entry added to README.md to specify static tag usage.
  FYI @pmcollins

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Konig-Corey (88dccbc2fd16bcc3166f8c31022513c97e638373, d92385ce68935a199fb37f07010724d412003681, e8bc25c8b9df6dd533938da6c0c7fcbfe4af8881)

[dmitryax]

Also it would be nice to have this PR broken into several ones since the fixes are unrelated to each other. And don't forget to add change log entries :)

[Konig-Corey]

I can close this and split it into two. I agree it makes more sense to have two PRs one for the addition of static tags, and another for the timestamp addition.

[Konig-Corey]

@dmitryax removed the timestamp fix and will move to a different PR. Leaving this open for addition of static tag feature.

[dmitryax]

@Konig-Corey please update PR description

[dmitryax]

and a changelog entry is still required

[dmitryax]

We just recently introduced another way to handle changelog. Please follow the updated process https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-a-changelog-entry

[Konig-Corey]

@dmitryax Added new change log.

[dmitryax]

@Konig-Corey please rebase

[Konig-Corey]

@Konig-Corey please rebase

rebased from upstream main branch.

[dmitryax]

please take a look at CI failures

[mx-psi]

What if I want to set an attribute to true? How will we add this if we need it in a backwards compatible way?

[Konig-Corey]

Im not sure what you are asking. It only takes string values. Any non string values would likely cause an issue or be converted to a string.

[dmitryax]

The @mx-psi's point is that if we want to add attributes with different value types, we won't be able to do that without breaking existing configuration interface. So it's better to design it better in advance. Maybe something like:

static_attributes: 
  - key: dbinstance
    string_value: mydbinstance

[dmitryax]

Are we still applying this configuration interface ^ ? @mx-psi what do you think bout it?

[mx-psi]

I think changing StaticAttributes to map[string]interface{} and then doing:

for k, v := range cfg.StaticAttributes {
     switch v.(type) {
         case string:
	     attrs.InsertString(k, v)
          // ... other cases here, or even just a default that returns an error saying non-string is not supported
     }

Should work, and we don't have to complicate the configuration schema. Does that make sense?

[mx-psi]

Our config provider definitely allows doing that: https://go.dev/play/p/CsyC3KuTer5

The yes/no case is an interesting edge case. The YAML package we use documents this explicitly here, in this case it will be a string since the field we unmarshal into is not a bool (see playground above). I think that's an acceptable behavior for me (but I understand if you feel otherwise).

[dmitryax]

I'm ok with this approach. @Konig-Corey can you please keep existing interface but make sure that only string values are accepted for now. Later we can add support for other types if needed

[dmitryax]

After giving it a second thought, I would say I'd rather prefer explicit type definition in yaml config rather than relying on implying type from the value. As a user, I would rather be explicit about what type of values I will get, instead of figuring out what's the parsing logic to make sure that attribute has a type that I want. I don't think users will be setting many static attributes, usually one or two per metric, having another line in the settings per attribute is not a big overhead IMO.

Specifying attributes in a list also makes the emitted metric with attributes in predictable order.

[mx-psi]

I guess not everyone has the same familiarity with YAML's quirkiness so that's a good argument.

Another reason I did not mention before to favor the map approach is that we sort-of have that on core here. I don't feel very strongly in favor of either implementation, but we should probably be consistent on both places. I don't want to drag this on forever, so I am fine with trying the explicit type here and then deciding which one we like the most.

[dmitryax]

Makes sense, let's keep the same approach in this processor. I believe we can keep existing config behavior, given that the component is in alpha state. We can add restriction to string only attribute later.

[mx-psi]

Looks like a rebase went wrong?

[Konig-Corey]

Working to fix it. Sorry about that.

[dmitryax]

LGTM. Sorry for the delay

[dmitryax]

@mx-psi PTAL