Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sqlqueryreceiver] - add optional static_attributes to metrics. #11868

Merged
merged 9 commits into from
Jul 27, 2022
Merged

[sqlqueryreceiver] - add optional static_attributes to metrics. #11868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
43408ec
add timestamp to generated metrics
Konig-Corey Jul 1, 2022
afbbb82
add support for static tags
Konig-Corey Jul 1, 2022
65f18ce
move timestamp fix to new PR.
Konig-Corey Jul 1, 2022
bc0c95e
tags -> static_attributes, add changelog
Konig-Corey Jul 1, 2022
7cb2c95
update readme verbage for static attributes
Konig-Corey Jul 5, 2022
947b15a
add new change log
Konig-Corey Jul 5, 2022
bf6af71
readme verbage.
Konig-Corey Jul 5, 2022
3e0e7e3
add PR to requried field
Konig-Corey Jul 5, 2022
eafd969
chnage log syntax
Konig-Corey Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions receiver/sqlqueryreceiver/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Each _metric_ in the configuration will produce one OTel metric per row returned
* `aggregation` (optional): only applicable for `data_type=sum`; can be `cumulative` or `delta`; defaults to `cumulative`.
* `description` (optional): the description applied to the metric.
* `unit` (optional): the units applied to the metric.
* `static_attributes` (optional): static attributes applied to the metrics
mx-psi marked this conversation as resolved.
Show resolved Hide resolved

### Example

Expand All @@ -44,6 +45,8 @@ receivers:
- metric_name: movie.genres
value_column: "count"
attribute_columns: [ "genre" ]
static_attributes:
dbinstance: mydbinstance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Konig-Corey Konig-Corey Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dmitryax dmitryax Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@dmitryax dmitryax Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

```

Given a `movie` table with three rows:
Expand Down Expand Up @@ -72,6 +75,7 @@ Descriptor:
NumberDataPoints #0
Data point attributes:
-> genre: STRING(sci-fi)
-> dbinstance: STRING(mydbinstance)
Value: 2

Metric #1
Expand All @@ -81,5 +85,6 @@ Descriptor:
NumberDataPoints #0
Data point attributes:
-> genre: STRING(action)
-> dbinstance: STRING(mydbinstance)
Value: 1
```
1 change: 1 addition & 0 deletions receiver/sqlqueryreceiver/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ type MetricCfg struct {
Aggregation MetricAggregation `mapstructure:"aggregation"`
Unit string `mapstructure:"unit"`
Description string `mapstructure:"description"`
StaticAttributes map[string]string `mapstructure:"static_attributes"`
}

func (c MetricCfg) Validate() error {
Expand Down
1 change: 1 addition & 0 deletions receiver/sqlqueryreceiver/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func TestParseConfig(t *testing.T) {
assert.Equal(t, false, metric.Monotonic)
assert.Equal(t, MetricDataTypeSum, metric.DataType)
assert.Equal(t, MetricValueTypeInt, metric.ValueType)
assert.Equal(t, map[string]string{"foo": "bar"}, metric.StaticAttributes)
assert.Equal(t, MetricAggregationCumulative, metric.Aggregation)
}

Expand Down
3 changes: 3 additions & 0 deletions receiver/sqlqueryreceiver/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ func rowToMetric(row metricRow, cfg MetricCfg, dest pmetric.Metric) error {
return fmt.Errorf("rowToMetric: attribute_column not found: '%s'", columnName)
}
}
for k, v := range cfg.StaticAttributes {
attrs.InsertString(k, v)
}
return nil
}

Expand Down
2 changes: 2 additions & 0 deletions receiver/sqlqueryreceiver/testdata/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ receivers:
value_type: int
monotonic: false
aggregation: cumulative
static_attributes:
foo: bar
exporters:
nop:
service:
Expand Down
11 changes: 11 additions & 0 deletions unreleased/sqlqueryreceiver-tags.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: sqlqueryreceiver

# A brief description of the change
note: Add static_attributes optional configuration to allow adding attributes/tags to queried metrics.

# One or more tracking issues related to the change
issues: [11868]