-
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
OpenSearch Exporter #23045
OpenSearch Exporter #23045
Conversation
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
f8a1475
to
972f8b2
Compare
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
10e4b87
to
df4a1e8
Compare
Signed-off-by: MaxKsyunz <[email protected]>
df4a1e8
to
a24c245
Compare
Signed-off-by: MaxKsyunz <[email protected]>
63be6d9
to
4d90f42
Compare
exporter/opensearchexporter/model.go
Outdated
// encodeModel tries to keep the event as close to the original open telemetry semantics as is. | ||
// No fields will be mapped by default. | ||
// | ||
// Field deduplication and dedotting of attributes is supported by the encodeModel. |
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.
What is dedotting?
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.
Have a look at exporter/opensearchexporter/internal/objmodel/objmodel.go. This explains a lot about it. In short, this is something, that is required for OpenSearch due to its interpretation of dots in field names (keys).
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.
Thanks for this great contribution. It seems you have implemented traces and logs. Can you adjust the README.md to reflect this?
I am concerned about the compatibility between the data generated with this exporter when compared with using DataPrepper for the same job. There is real nice support for traces in OpenSearch Dashboards with the observability plugin, which builds on the data model provided by DataPrepper. Will this be useable with the OpenTelemetry OpenSearch exporter as well?
A particular feature of the DataPrepper approach we like, is the common handling of attributes and resource attributes, which can be shared between traces and logs. Having the name schema allows to create dashboards and visualisations in OpenSearch feeding from different indices for logs, traces, and metrics utilising common filters. This should also be able with the exported of this PR. As I understand it, the different naming schemes will render indices created by the OpenTelemetry Collector and DataPrepper incompatible with regards to filtering, e.g. by trace id or service name. Can this be improved?
- `traces_index`: The | ||
[index](https://opensearch.org/docs/latest/opensearch/index-data/) | ||
or [datastream](https://opensearch.org/docs/latest/opensearch/data-streams/) | ||
name to publish trace events to. The default value is `traces-generic-default`. Note: To better differentiate between log indexes and traces indexes, `index` option are deprecated and replaced with below `logs_index` |
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.
Why does this not point to otel-v1-apm-span-000001
or similar, which would be used by DataPrepper to provide data for the OpenSearch Observability Trace plugin? Is there some kind of incompatibility between the data?
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.
data-prepper is currently also undergoing the movement to data-stream so that both ingestion concepts would use the same mechanism
In addition the catalog repository recently added enables the consolidation and sharing of knowledge and physical mapping for all the plugins across the org
| Status | | | ||
| ------------------------ |---------------| | ||
| Stability | [development] | | ||
| Supported pipeline types | traces | |
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.
The example below contains a configuration for logs besides the traces pipeline. How does this relate to this field, which states, that only traces is supported?
- `read_buffer_size` (default=0): Read buffer size. | ||
- `write_buffer_size` (default=0): Write buffer size used when. | ||
- `timeout` (default=90s): HTTP request time limit. | ||
- `headers` (optional): Headers to be send with each HTTP request. |
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.
Does the exported use compression, e.g. gzip, by default? Can this be added as a setting?
|
||
// Headers allows users to configure optional HTTP headers that | ||
// will be send with each HTTP request. | ||
Headers map[string]string `mapstructure:"headers,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.
Wouldn't it be better to use a secure string for the header values, as they could contain sensitive information similar to the password of the AuthenticationSettings?
- `mapping`: Events are encoded to JSON. The `mapping` allows users to | ||
configure additional mapping rules. |
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.
Config validation looks for particular values none
, no
, and sso
. Can you add documentation for these values, so that it is clear, what they mean?
}, | ||
want: Document{[]field{{"a", IntValue(1)}, {"z", IntValue(26)}}}, | ||
}, | ||
"sorting is stable": { |
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 a required quality, as long as the sorting provides the same result given the same input?
exporter/opensearchexporter/model.go
Outdated
// encodeModel tries to keep the event as close to the original open telemetry semantics as is. | ||
// No fields will be mapped by default. | ||
// | ||
// Field deduplication and dedotting of attributes is supported by the encodeModel. |
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.
Have a look at exporter/opensearchexporter/internal/objmodel/objmodel.go. This explains a lot about it. In short, this is something, that is required for OpenSearch due to its interpretation of dots in field names (keys).
exporter/opensearchexporter/model.go
Outdated
document.AddTimestamp("@timestamp", span.StartTimestamp()) // We use @timestamp in order to ensure that we can index if the default data stream logs template is used. | ||
document.AddTimestamp("EndTimestamp", span.EndTimestamp()) | ||
document.AddTraceID("TraceId", span.TraceID()) | ||
document.AddSpanID("SpanId", span.SpanID()) | ||
document.AddSpanID("ParentSpanId", span.ParentSpanID()) | ||
document.AddString("Name", span.Name()) | ||
document.AddString("Kind", traceutil.SpanKindStr(span.Kind())) | ||
document.AddInt("TraceStatus", int64(span.Status().Code())) | ||
document.AddString("Link", spanLinksToString(span.Links())) | ||
document.AddAttributes("Attributes", span.Attributes()) | ||
document.AddAttributes("Resource", resource.Attributes()) |
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 looks very different to the structure DataPrepper would generate. This is important, since the OpenSearch Observability Trace Analytics build on that data model. Here is an example from an OpenSearch index:
"_source": {
"kind": "SPAN_KIND_INTERNAL",
"traceId": "eaa69f00b4ab63fd618eadffb00e60d7",
"spanId": "75d7d735cb2b4972",
"parentSpanId": "84f4ad4086932693",
"traceState": "",
"traceGroup": null,
"serviceName": "sample",
"name": "LogController.generateLog",
"startTime": "2023-06-09T14:32:34.444486691Z",
"endTime": "2023-06-09T14:32:34.446411315Z",
"durationInNanos": 1924624,
"status.code": 0,
"events": [],
"droppedEventsCount": 0,
"traceGroupFields": {
"endTime": null,
"durationInNanos": null,
"statusCode": null
},
"links": [],
"droppedLinksCount": 0,
"instrumentationScope.version": "1.25.0-alpha",
"instrumentationScope.name": "io.opentelemetry.spring-webmvc-3.1",
"resource.attributes.process@pid": 7,
"resource.attributes.host@arch": "amd64",
"resource.attributes.telemetry@sdk@version": "1.25.0",
"resource.attributes.service@name": "sample",
"resource.attributes.process@runtime@name": "Java(TM) SE Runtime Environment",
"resource.attributes.os@type": "linux",
"resource.attributes.telemetry@sdk@language": "java",
"resource.attributes.host@name": "18232480-822d-40e2-5ded-ebb2",
"resource.attributes.process@runtime@version": "8.1.094",
"resource.attributes.telemetry@sdk@name": "opentelemetry",
"resource.attributes.telemetry@auto@version": "1.25.0",
"resource.attributes.os@description": "Linux 5.19.0-41-generic",
"span.attributes.thread@id": 36,
"span.attributes.thread@name": "http-nio-8080-exec-8",
"droppedAttributesCount": 0
}
How does the two naming schemes align in OpenSearch. Is there already support for both?
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.
@KarstenSchnitter yes this was based on the elastic exporter. I will be changing the schema shortly.
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.
@KarstenSchnitter could you please open a DataPrepper issue to make any necessary changes?
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.
@kkondaka what would be the concrete changes required in DataPrepper? It is not clear to me, what naming would be preferable. Some are just capitalization, some are specific to DataPrepper (extra serviceName and traceGroup) and others dilute understanding of the original OTel fields like @timestamp
and TraceStatus
(to a lesser extend). Attributes
is in plural, where Link
and Resource
are not.
I would prefer to not change the names in DataPrepper, since we use it in production. If this is necessary for alignment between the two tools, so be it.
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.
@KarstenSchnitter we definitely do not want to make any changes that would break any existing deployment. We may want to add new fields and slowly deprecate old ones
|
||
case resp.Status == 0 && err != nil: | ||
// Encoding error. We didn't even attempt to send the event | ||
logger.Error("Drop docs: failed to add docs to the bulk request buffer.", |
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 am not sure on the severity of dropped docs. Since the collector remains running and connecting to OpenSearch, I would see this with a lower severity. My thinking is, that dropping some of the docs is less severe as dropping all of them due to misconfiguration or network partition. Especially indexing errors (further down) can occur due to conflicts within a particular document.
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.
Does this instrumentation expose metrics on the number of dropped docs? This would really help observability of the Collector.
return multierr.Combine(errs...) | ||
} | ||
|
||
func (e *opensearchTracesExporter) pushTraceRecord(ctx context.Context, resource pcommon.Resource, span ptrace.Span) error { |
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.
Do you plan adding support for service maps as well?
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.
Service map is not planned currently in this exporter - we are thinking of using the existing OTEL contribute service graph processor and consolidate that functionality with data prepper
Note that this component has NOT been accepted yet. There are no sponsors for this component. Until you find an sponsor who can commit to review and co-own this component, this is not going to be accepted. |
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
…porter Signed-off-by: MaxKsyunz <[email protected]> # Conflicts: # cmd/otelcontribcol/builder-config.yaml # cmd/otelcontribcol/go.mod # cmd/otelcontribcol/go.sum # cmd/oteltestbedcol/builder-config.yaml # cmd/oteltestbedcol/go.mod # cmd/oteltestbedcol/go.sum
Yes, very true. How can I get in touch with maintainer group to begin looking for a sponsor? |
fc4e8d2
to
96e322d
Compare
Thank you for a thorough review @KarstenSchnitter. Yes, this schema is odd -- I'll replace it shortly. FWIW I started with it because that's what The end-goal of this PR is to export otel signals to OpenSearch in a schema compatible with OpenTelemetry standard. The specific schema is here. This is part of a larger effort to improve OpenSearch Observability plugin. See here for details. |
switch candidate { | ||
case "none": | ||
case "no": | ||
case "sso": |
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.
We changed the naming convention from SSO to SS4O to avoid confusion with single sign on, can we change all the references to SS4O?
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.
Yep, coming up.
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.
@MaxKsyunz plz update to the current location of the Observability mapping schema
…porter # Conflicts: # cmd/otelcontribcol/go.mod # cmd/otelcontribcol/go.sum
Signed-off-by: MaxKsyunz <[email protected]>
@KarstenSchnitter has raised many important concerns regarding compatibility between simple schema and data-prepper traces. Recently @kkondaka and myself worked together for data-prepper & SimpleSchema to share the same structure for Metrics (this has already been released in the latest data-prepper version). Regarding logs - the recent announcement by OTEL and ECS for converging both protocols into one is a large step forward that we at OpenSearch would very much like to adopt. This is why we are currently postponing the Logs support in the exporter until OpenTelemetry will release the first stable version we can reference and follow. This will be probably true for data-prepper as well and therefore I think we need to continue our current work on traces and wait for the consolidated logs protocol to be published. Regarding Observability-Dashboards and trace analytics dialog - we are adding support for custom selection of trace index source so that the user can select the source of the trace data. Since there is already a large overlap between data-prepper and simple schema - they both would work similarly |
Isn't the ECS merger only about semantic conventions? I think that work has completed already. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Continued in separate PRs as per contributor guidelines. |
Adding initial set-up for OpenSearch exporter addition. Future PRs will include adding functionality to the exporter, code coverage and e2e tests. Broken up for easier review. [New component proposal.](#23611) Start of resolution of #7905. Will come in future PR to 80+% coverage. Break-up of #23045 --------- Signed-off-by: Max Ksyunz <[email protected]>
Adding collector exporter for OpenSearch. Currently only sends traces to OpenSearch. Supports simple schema for observability.
Here's a demo of it all working together using
examples/demo
app.OpenSearch.Otel.Exporter.mp4
References
Resolves #7905.