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

[receiver/prometheusreceiver] Add version and name to metrics #20903

Merged
merged 2 commits into from
Apr 17, 2023
Merged

[receiver/prometheusreceiver] Add version and name to metrics #20903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41021cd
[receiver/prometheusreceiver] Add version and name to metrics
paologallinaharbur Apr 14, 2023
d3b85ff
Adding unit tests
paologallinaharbur Apr 14, 2023
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
16 changes: 16 additions & 0 deletions .chloggen/prometheus-version-name-in-metrics.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# 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: prometheusreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: All receivers are setting receiver name and version when sending data. This change introduces the same behaviour to the prometheus receiver.

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

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
9 changes: 8 additions & 1 deletion receiver/prometheusreceiver/internal/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/prometheus/prometheus/model/value"
"github.com/prometheus/prometheus/scrape"
"github.com/prometheus/prometheus/storage"
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/featuregate"
"go.opentelemetry.io/collector/obsreport"
Expand All @@ -41,6 +42,7 @@ import (

const (
targetMetricName = "target_info"
receiverName = "otelcol/prometheusreceiver"
)

type transaction struct {
Expand All @@ -52,6 +54,7 @@ type transaction struct {
externalLabels labels.Labels
nodeResource pcommon.Resource
logger *zap.Logger
buildInfo component.BuildInfo
metricAdjuster MetricsAdjuster
obsrecv *obsreport.Receiver
// Used as buffer to calculate series ref hash.
Expand All @@ -75,6 +78,7 @@ func newTransaction(
metricAdjuster: metricAdjuster,
externalLabels: externalLabels,
logger: settings.Logger,
buildInfo: settings.BuildInfo,
obsrecv: obsrecv,
bufBytes: make([]byte, 0, 1024),
normalizer: prometheustranslator.NewNormalizer(registry),
Expand Down Expand Up @@ -207,7 +211,10 @@ func (t *transaction) getMetrics(resource pcommon.Resource) (pmetric.Metrics, er
md := pmetric.NewMetrics()
rms := md.ResourceMetrics().AppendEmpty()
resource.CopyTo(rms.Resource())
metrics := rms.ScopeMetrics().AppendEmpty().Metrics()
ils := rms.ScopeMetrics().AppendEmpty()
ils.Scope().SetName(receiverName)
Copy link
Member

@mwear mwear Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You could consider adding a scope field to the transaction struct, and initializing the it in newTransaction. Then you could copy the scope here instead of recreating it each time. For example:

type transaction struct {
  ...
  scope          pcommon.InstrumentationScope
  ...

func newTransaction() {
  ...
  scope := pcommon.NewInstrumentationScope()
  scope.SetName(receiverName)
  scope.SetVersion(buildInfo.Version)
  ...
}

then here you can:

ils := rms.ScopeMetrics().AppendEmpty()
t.scope.CopyTo(ils.Scope())

Copy link
Member Author

@paologallinaharbur paologallinaharbur Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hello! That would be another option, however, the implementation would be different from the one of any other receiver. Do you think that there is a performance improvement justifying a different implementation? 😄

As far as I understand .Scope() does not create a new scope, but it returns the corresponding one to set Name and Versions.

It is true that a copy is just one action, and setting Name and Version would be two, but the difference should be negligible, and the library of the copy is calling as well SetName and SetVersion under the hood.

func (ms InstrumentationScope) CopyTo(dest InstrumentationScope) {
	dest.SetName(ms.Name())
	dest.SetVersion(ms.Version())

Copy link
Member Author

Choose a reason for hiding this comment

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

Currrent implementation

BenchmarkReceiverVersionAndNameAreAttached-10    	  302818	      4001 ns/op

Proposed one

BenchmarkReceiverVersionAndNameAreAttached-10    	  304873	      4082 ns/op

Copy link
Member

Choose a reason for hiding this comment

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

It seems negligible at best. I'm happy with the current implementation.

ils.Scope().SetVersion(t.buildInfo.Version)
metrics := ils.Metrics()

for _, mf := range t.families {
mf.appendMetric(metrics, t.normalizer)
Expand Down
22 changes: 22 additions & 0 deletions receiver/prometheusreceiver/internal/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,28 @@ func TestTransactionAppendResource(t *testing.T) {
require.Equal(t, expectedResource, gotResource)
}

func TestReceiverVersionAndNameAreAttached(t *testing.T) {
sink := new(consumertest.MetricsSink)
tr := newTransaction(scrapeCtx, &startTimeAdjuster{startTime: startTimestamp}, sink, nil, receivertest.NewNopCreateSettings(), nopObsRecv(t), featuregate.GlobalRegistry())
_, err := tr.Append(0, labels.FromMap(map[string]string{
model.InstanceLabel: "localhost:8080",
model.JobLabel: "test",
model.MetricNameLabel: "counter_test",
}), time.Now().Unix()*1000, 1.0)
assert.NoError(t, err)
assert.NoError(t, tr.Commit())

expectedResource := CreateResource("test", "localhost:8080", labels.FromStrings(model.SchemeLabel, "http"))
mds := sink.AllMetrics()
require.Len(t, mds, 1)
gotResource := mds[0].ResourceMetrics().At(0).Resource()
require.Equal(t, expectedResource, gotResource)

gotScope := mds[0].ResourceMetrics().At(0).ScopeMetrics().At(0).Scope()
require.Equal(t, receiverName, gotScope.Name())
require.Equal(t, component.NewDefaultBuildInfo().Version, gotScope.Version())
}

func TestTransactionCommitErrorWhenAdjusterError(t *testing.T) {
goodLabels := labels.FromMap(map[string]string{
model.InstanceLabel: "localhost:8080",
Expand Down