-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[exporter/opensearch] Send logs to Opensearch #26475
[exporter/opensearch] Send logs to Opensearch #26475
Conversation
5c1bae6
to
2e579e0
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
54e12e8
to
59e1793
Compare
@Aneurysm9 @bryan-aguilar friendly pinging for review here so the PR isn't closed for inactivity. Thanks! |
Pinging @MitchellGale @MaxKsyunz @YANG-DB as opensearch exporter codeowners. |
i have ran the integration tests, please check their success status and respond to any failures. |
e714108
to
9001616
Compare
There were some issues regarding the changelog that should be addressed now. Can you run them again, please? Thanks! |
This PR would be extremely useful for our use case as the client in the elasticsearch exporter blocks working with opensearch clusters. If there's anything I can do to help push this PR across the finish line, please let me know. It looks like it's code complete and the tests are passing. |
This PR is exactly what we need to adopt this package for an AWS solutions project. @Aneurysm9 is there anything that is preventing this PR from being merged? |
This PR is still awaiting approval from code owners @sumobrian |
Tagging code owners: @Aneurysm9 @MitchellGale @MaxKsyunz @YANG-DB |
Thanks for your support |
9001616
to
cf3ac2c
Compare
@atoulme @YANG-DB @bryan-aguilar rebased to fix conflicts. It should be good to go now. |
@Aneurysm9 - can you please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed primarily for structure and consistency with collector style and expectations. The notes here are not blocking as they do not affect the public API, but should be addressed before moving out of the development
stage.
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package opensearchexporter // import "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/opensearchexporter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing here is exported, which is a good thing. Please consider moving this to an internal
package to minimize the likelihood of accidental changes to visibility increasing the public API surface area.
return exporterhelper.NewLogsExporter(ctx, set, cfg, | ||
le.pushLogData, | ||
exporterhelper.WithStart(le.Start), | ||
exporterhelper.WithCapabilities(consumer.Capabilities{MutatesData: true}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? If the exporter can avoid mutating data it can save a copy operation if it is used in a pipeline with multiple exporters.
// Ingest Node is used. But either way, we try to present only well formed | ||
// document to OpenSearch. | ||
|
||
// nolint:errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be scoped to specific statements? Even if that requires duplication it's preferable to not allowing further unchecked errors to be introduced without discussion.
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package opensearchexporter // import "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/opensearchexporter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may also be a candidate for moving to an internal package.
Links []ssoSpanLinks `json:"links,omitempty"` | ||
Name string `json:"name"` | ||
ParentSpanID string `json:"parentSpanId"` | ||
Resource map[string]string `json:"resource,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource attributes can be more than just strings. How does this handle non-string-valued resource attributes?
For note: I will address the comments in another PR, including a refactor in the bulk indexer structure. |
Hi |
An @open-telemetry/collector-contrib-maintainer will come by and merge it when they get a chance. It is not done automatically. |
@jaehnri, please address merge conflicts though as that will prevent a maintainer from merging it in. |
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
…space Signed-off-by: João Henri <[email protected]>
Signed-off-by: João Henri <[email protected]>
…bility Signed-off-by: João Henri <[email protected]>
1d2d4a9
to
85b840c
Compare
Signed-off-by: João Henri <[email protected]>
@bryan-aguilar, done it. |
@jaehnri can you create a tracking issue for addressing the changes in @Aneurysm9's review? I will merge this now, but please link the issue here once you have created it :) |
## Description: <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> Implementation of exporter to OpenSearch using opensearch-go library. As of now, this PR was heavily inspired by https://github.com/dbason/opentelemetry-collector-contrib/tree/opensearch-exporter/exporter/opensearchexporter. By default, requests sent adhere to the OpenSearch Catalog [schema for logs](https://github.com/opensearch-project/opensearch-catalog/tree/main/schema/observability/logs), but allows users to export using the Elastic Common Schema as well. This PR also: - enables users to define the `bulk_action` between `create` and `index` - enables users to define the logs index without necessarily adhering to the new [index naming conventions](opensearch-project/observability#1405) through the `LogsIndex` config. ## Tracking Issue: [23611](open-telemetry#23611) ## Testing: <Describe what testing was performed and which tests were added.> ### Integration - Successful round-trip to HTTP endpoint, - Permanent error during round-trip, - Retryable error response for first request, followed by successful response on retry, - Two retriable error responses, followed by successful response on second retry. ### Manual - Authentication using `configtls.TLSSetting` (`ca_file`, `cert_file`, `key_file`) - Tested in EKS and K3s clusters running [opni](https://github.com/rancher/opni). --------- Signed-off-by: João Henri <[email protected]> Signed-off-by: João Henri <[email protected]>
…8662) This fixes a merge conflict between open-telemetry#28594 and open-telemetry#26475
## Description: <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> Implementation of exporter to OpenSearch using opensearch-go library. As of now, this PR was heavily inspired by https://github.com/dbason/opentelemetry-collector-contrib/tree/opensearch-exporter/exporter/opensearchexporter. By default, requests sent adhere to the OpenSearch Catalog [schema for logs](https://github.com/opensearch-project/opensearch-catalog/tree/main/schema/observability/logs), but allows users to export using the Elastic Common Schema as well. This PR also: - enables users to define the `bulk_action` between `create` and `index` - enables users to define the logs index without necessarily adhering to the new [index naming conventions](opensearch-project/observability#1405) through the `LogsIndex` config. ## Tracking Issue: [23611](open-telemetry#23611) ## Testing: <Describe what testing was performed and which tests were added.> ### Integration - Successful round-trip to HTTP endpoint, - Permanent error during round-trip, - Retryable error response for first request, followed by successful response on retry, - Two retriable error responses, followed by successful response on second retry. ### Manual - Authentication using `configtls.TLSSetting` (`ca_file`, `cert_file`, `key_file`) - Tested in EKS and K3s clusters running [opni](https://github.com/rancher/opni). --------- Signed-off-by: João Henri <[email protected]> Signed-off-by: João Henri <[email protected]>
…8662) This fixes a merge conflict between open-telemetry#28594 and open-telemetry#26475
Description:
Implementation of exporter to OpenSearch using opensearch-go library. As of now, this PR was heavily inspired by https://github.com/dbason/opentelemetry-collector-contrib/tree/opensearch-exporter/exporter/opensearchexporter.
By default, requests sent adhere to the OpenSearch Catalog schema for logs, but allows users to export using the Elastic Common Schema as well.
This PR also:
bulk_action
betweencreate
andindex
LogsIndex
config.Tracking Issue:
23611
Testing:
Integration
Manual
configtls.TLSSetting
(ca_file
,cert_file
,key_file
)