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

Create pkg/datadog module #35067

Merged

Conversation

dineshg13
Copy link
Member

@dineshg13 dineshg13 commented Sep 8, 2024

Description:

The PR creates pkg/datadog package with configuration for DD Exporter and DD connector.
The PR currently retains tests in exporter/datadogexporter & connector/datadogconnector . These will be remove along with the configuration in exporter & connector.

@dineshg13 dineshg13 marked this pull request as ready for review September 8, 2024 19:57
@dineshg13 dineshg13 requested a review from a team September 8, 2024 19:57
@dineshg13 dineshg13 changed the title Dinesh.gurumurthy/datadog config pkg Create pkg/datadog module Sep 8, 2024
connector/datadogconnector/config_test.go Show resolved Hide resolved
exporter/datadogexporter/config.go Outdated Show resolved Hide resolved
pkg/datadog/host.go Outdated Show resolved Hide resolved
exporter/datadogexporter/traces_exporter.go Outdated Show resolved Hide resolved
exporter/datadogexporter/integrationtest/go.mod Outdated Show resolved Hide resolved
pkg/datadog/traces.go Outdated Show resolved Hide resolved
pkg/datadog/traces.go Outdated Show resolved Hide resolved
pkg/datadog/traces.go Outdated Show resolved Hide resolved
pkg/datadog/traces.go Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Sep 10, 2024

Can you fix the merge conflict?

.chloggen/dinesh.gurumurthy_datadog-config-pkg.yaml Outdated Show resolved Hide resolved
.chloggen/dinesh.gurumurthy_datadog-config-pkg.yaml Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
Co-authored-by: Yang Song <songy23@users.noreply.github.com>
Comment on lines 107 to 110
// AddWarning appends to warnings in the configuration.
func (c *Config) AddWarning(warning error) {
c.warnings = append(c.warnings, warning)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to make this method public? I want to keep as much of this 'hack' to log warnings private

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

// SourceTimeout is the timeout to fetch from each provider - for example AWS IMDS.
// If unset, or set to zero duration, there will be no timeout applied.
// Default is no timeout.
SourceTimeout time.Duration `mapstructure:"source_timeout"`
Copy link
Member

Choose a reason for hiding this comment

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

We are making this public, can we still keep it private somehow? If not, I would like to

  • Document this in our examples
  • Add a release note
  • Make sure this is properly designed and validated so users can't misuse it

Copy link
Member Author

Choose a reason for hiding this comment

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

Made it private with set & getter methods

@songy23
Copy link
Member

songy23 commented Sep 11, 2024

Needs make gotidy too

@songy23
Copy link
Member

songy23 commented Sep 11, 2024

@dineshg13
Copy link
Member Author

@songy23 fixed.

@mx-psi mx-psi merged commit 9ae1a0f into open-telemetry:main Sep 12, 2024
155 of 156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 12, 2024
@crobert-1 crobert-1 mentioned this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/otelcontribcol otelcontribcol command connector/datadog exporter/datadog Datadog components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants