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.

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

[chore] Support connector in metrics.go.tmpl NewMetricsBuilder #12403

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Dainerx
Copy link
Contributor

@Dainerx Dainerx commented Feb 17, 2025

Description

I discovered this issue while trying to upgrade the opentelemetry collector from v0.111.0 to v0.119.0 that was released two weeks ago. To reproduce please take a look at the issue below.

Link to tracking issue

Fixes #12402

Testing

Run make generate on a processor or connector component

@Dainerx Dainerx requested review from dmitryax and a team as code owners February 17, 2025 10:29
@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch from c7f4818 to bb83847 Compare February 17, 2025 10:35
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 82.60870% with 16 lines in your changes missing coverage. Please review.

Project coverage is 92.15%. Comparing base (56fbf4c) to head (247a505).

Files with missing lines Patch % Lines
cmd/mdatagen/internal/sampleconnector/factory.go 40.00% 6 Missing ⚠️
...pleconnector/internal/metadata/generated_config.go 83.78% 4 Missing and 2 partials ⚠️
...econnector/internal/metadata/generated_resource.go 91.11% 4 Missing ⚠️

❌ Your patch check has failed because the patch coverage (82.60%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12403      +/-   ##
==========================================
- Coverage   92.18%   92.15%   -0.04%     
==========================================
  Files         465      468       +3     
  Lines       25275    25367      +92     
==========================================
+ Hits        23301    23377      +76     
- Misses       1575     1589      +14     
- Partials      399      401       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch from bb83847 to 4355c04 Compare February 17, 2025 16:22
@bogdandrutu
Copy link
Member

@Dainerx what other component do you have that emits metrics?

@Dainerx
Copy link
Contributor Author

Dainerx commented Feb 17, 2025

@bogdandrutu in my use case, processors and connectors do emit metrics.

My main concern is the if statement surrounding only the function signature. Also, I expected adding a new class in the metadata schema.

@bogdandrutu
Copy link
Member

My main concern is the if statement surrounding only the function signature. Also, I expected adding a new class in the metadata schema.

That should be fixed.

processors and connectors do emit metrics.

How come? I can see why connectors may do it, but how come the processors?

@Dainerx
Copy link
Contributor Author

Dainerx commented Feb 18, 2025

How come? I can see why connectors may do it, but how come the processors?

Processors can have mutate data capabilities, the metrics generation processor does generate metrics. Although, this is possible, I agree with you it's not a best practice to use a processor to generate metrics.

I am fine with only supporting connector scrapper, and receiver. We should also:

  1. Add a sample connector to be used for testing purposes to check the output of mdatagen against it
  2. Update the README file.

WDYT?

@Dainerx Dainerx changed the title [chore] Remove conditional statement on metrics.go.tmpl NewMetricsBuilder [chore] Support connector in metrics.go.tmpl NewMetricsBuilder Feb 18, 2025
@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch from 4355c04 to 9dded0f Compare February 18, 2025 12:28
@bogdandrutu
Copy link
Member

I am ok with this proposal.

@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch 2 times, most recently from 0a0f205 to 7d5fcd2 Compare February 18, 2025 16:57
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I am not sure what changed, but this looks very strange that every line is updated.

@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch from 7d5fcd2 to 31d7313 Compare February 20, 2025 11:46
@Dainerx Dainerx force-pushed the fix/cmd/mdatada-gen-metrics branch from 31d7313 to 54a8c7c Compare February 20, 2025 12:14
@@ -17,7 +17,7 @@ Examples of this metadata about a component are:
* its stability level
* the distributions containing it
* the types of pipelines it supports
* metrics emitted in the case of a scraping receiver
* metrics emitted in the case of a scraping receiver, a scrapper, or a connector
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* metrics emitted in the case of a scraping receiver, a scrapper, or a connector
* metrics emitted in the case of a scraping receiver, a scraper, or a connector

status:
class: connector
stability:
development: [logs]
Copy link
Contributor

Choose a reason for hiding this comment

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

it should look like metrics_to_metrics for connectors

"go.opentelemetry.io/collector/pdata/pmetric"
)

// NewFactory returns a receiver.Factory for sample receiver.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// NewFactory returns a receiver.Factory for sample receiver.
// NewFactory returns a connector.Factory for sample connector.

@@ -0,0 +1,67 @@
# Sample metadata file with all available configurations for a receiver.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Sample metadata file with all available configurations for a receiver.
# Sample metadata file with all available configurations for a connector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metadata generated_metrics compilation error (v119.0)
3 participants