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

Initial discovery receiver skeleton #1830

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Conversation

rmfitzpatrick
Copy link
Contributor

These changes introduce the initial Discovery receiver component methods and config validation (proposed readme: #1812).

@rmfitzpatrick rmfitzpatrick requested review from a team as code owners August 3, 2022 11:59
@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch from 3dbd6b3 to bdb0830 Compare August 3, 2022 12:24
@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch 2 times, most recently from c8fd33c to 632627d Compare August 16, 2022 19:19
}

func (lr *LogRecord) validate() error {
// TODO: supported severity text validation
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind if you want to use your own impl of LogRecord, but it might be easier to use plog.LogRecord if you want to access all the fields. For example, your LogRecord struct doesn't have SeverityText or SeverityNum. You would get that for free if you use plog.LogRecord.

Copy link
Contributor Author

@rmfitzpatrick rmfitzpatrick Aug 17, 2022

Choose a reason for hiding this comment

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

The config log record is used to express the values that are desired in the actual plog.LogRecord item that's going to be produced by the receiver. I don't know if it's possible to use a plog.LogRecord in its place given the implementation is in "go.opentelemetry.io/collector/pdata/internal/data/protogen/logs/v1" and doesn't use mapstructure, but considering we only care about specific fields with more convenient accessors I don't think it's a concern.

Severity becomes SeverityText but since we may want to specify num as well I can update it.*

edit: updated

@atoulme
Copy link
Contributor

atoulme commented Aug 16, 2022

Changes look ok:

  • the receiver is not mapped to components, so this is not going to affect the collector yet.
  • it would be good to have a README

@rmfitzpatrick
Copy link
Contributor Author

  • the receiver is not mapped to components, so this is not going to affect the collector yet.

I don't plan on adding this as a component until its initial functionality has been merged (observer event emitting and metrics/statement evaluation are next PRs). There are also some required upstream PRs for this to be fully functional:

open-telemetry/opentelemetry-collector-contrib#12801
open-telemetry/opentelemetry-collector-contrib#12670

  • it would be good to have a README

This was the first PR and has landed: https://github.com/signalfx/splunk-otel-collector/blob/main/internal/receiver/discoveryreceiver/README.md

@atoulme
Copy link
Contributor

atoulme commented Aug 17, 2022

  • the receiver is not mapped to components, so this is not going to affect the collector yet.

I don't plan on adding this as a component until its initial functionality has been merged (observer event emitting and metrics/statement evaluation are next PRs). There are also some required upstream PRs for this to be fully functional:
No worries.

  • it would be good to have a README

This was the first PR and has landed: https://github.com/signalfx/splunk-otel-collector/blob/main/internal/receiver/discoveryreceiver/README.md
I looked for it and missed it. Apologies!

@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch 2 times, most recently from fd7f2c8 to fb532cf Compare August 17, 2022 16:43
@rmfitzpatrick rmfitzpatrick requested a review from a team as a code owner August 17, 2022 16:43
@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch from fb532cf to 6db85b5 Compare August 17, 2022 18:51
Copy link
Contributor

@aurbiztondo-splunk aurbiztondo-splunk left a comment

Choose a reason for hiding this comment

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

Left a couple of comments!

@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch 2 times, most recently from 529c59c to 5edb839 Compare August 18, 2022 21:23
@rmfitzpatrick rmfitzpatrick force-pushed the discoveryreceiverskeleton branch from 5edb839 to b955816 Compare August 19, 2022 15:16
@rmfitzpatrick rmfitzpatrick merged commit f41a4cd into main Aug 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the discoveryreceiverskeleton branch August 22, 2022 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants