-
Notifications
You must be signed in to change notification settings - Fork 0
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
OpenTelemetry to OpenSearch Exporter #1
OpenTelemetry to OpenSearch Exporter #1
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]>
Signed-off-by: MaxKsyunz <[email protected]>
f743ba4
to
76e6c54
Compare
Signed-off-by: MaxKsyunz <[email protected]>
builder-config.yaml are config file for collector generator. go.mod/go.sum files needed to be updated as a consequence. Signed-off-by: MaxKsyunz <[email protected]>
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.
plz read the next PR's before addressing the comments
| Supported pipeline types | traces | | ||
| Distributions | [contrib] | | ||
|
||
This exporter supports sending OpenTelemetry logs to [OpenSearch](https://www.opensearch.org). |
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.
rename logs to signals as logs represent one type of the scope
- `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.
replace traces-generic-default
with `sso_traces-default-namespace'
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.
So SSO will be the default way that they store data? Should we have some kind of opt in variable? Will we be mapping to SSO by default?
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 does namespace mean in this context? Do you mean ${namespace} like qal, e2e, prod, etc.?
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.
So SSO will be the default way that they store data? Should we have some kind of opt in variable? Will we be mapping to SSO by default?
Good question. When exporter supports SSO, there will be a configuration setting. However, do we need to support the current schema? I created Bit-Quill/opentelemetry-collector-contrib#2 to discuss this question.
- `endpoints`: List of OpenSearch URLs. If endpoints is missing, the | ||
OPENSEARCH_URL environment variable will be used. | ||
- `num_workers` (optional): Number of workers publishing bulk requests concurrently. | ||
- `traces_index`: The |
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.
add similar notion for logs_index & metrics_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.
Do we need traces_index
, or logs_index
, or metrics_index
configuration options?
These options are unnecessary for SSO mode, because the destination is either derived from (type, dataset, namespace)
or traces-default-namespace
.
IMO If we support otel mode, then we keep traces_index
& co options. If we remove otel mode, then these options should be removed as well.
exporters: | ||
opensearch/trace: | ||
endpoints: [https://opensearch.example.com:9200] | ||
traces_index: trace_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.
add additional support for metrics
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.
Readme will be updated as part of Bit-Quill/opentelemetry-collector-contrib#3.
id: component.NewIDWithName(typeStr, "trace"), | ||
expected: &Config{ | ||
Endpoints: []string{"https://opensearch.example.com:9200"}, | ||
LogsIndex: "logs-generic-default", |
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.
change to traces-default-namespace
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.
sso_traces-default-${namespace}?
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.
Created Bit-Quill/opentelemetry-collector-contrib#4 to track decision.
const ( | ||
// The value of "type" key in configuration. | ||
typeStr = "opensearch" | ||
defaultLogsIndex = "logs-generic-default" |
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.
logs-default-namespace
traces-default-namespace
metrics-default-namespace
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 we want sso_ prefix? Coming from question in readme about whether we want sso to be default
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.
Created Bit-Quill/opentelemetry-collector-contrib#4 to track decision.
Timeout: 90 * time.Second, | ||
}, | ||
LogsIndex: defaultLogsIndex, | ||
TracesIndex: defaultTracesIndex, |
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.
add metrics support
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.
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
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.
remove comment?
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.
saw this in a few places
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.
It's for Go tooling.
TLDR: forces package users to use that URL to import this package.
} | ||
|
||
type HTTPClientSettings struct { | ||
Authentication AuthenticationSettings `mapstructure:",squash"` |
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 there a typo here?
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.
Sorry, what are you referring to? I don't see 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.
",squash" -> "squash", not sure why there is a leading comma
}, | ||
}, | ||
}, | ||
} |
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.
minor nit: do we want to add some test cases where mapping is not sso and assert error? Not too concerned though
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.
Supported mapping modes are "sso" and ("no", "none" or no value). In the later case it uses Otel-like schema.
I'll add a unit test for this case and created #8 to make sure unit test coverage is improved.
// createLogsExporter creates a new exporter for logs. | ||
// | ||
// Logs are directly indexed into OpenSearch. | ||
func createLogsExporter( |
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.
metrics exporter?
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.
Yes, when we add support for metrics -- Bit-Quill/opentelemetry-collector-contrib#3.
func TestFactory_CreateMetricsExporter_Fail(t *testing.T) { | ||
factory := NewFactory() | ||
cfg := factory.CreateDefaultConfig() | ||
params := exportertest.NewNopCreateSettings() | ||
_, err := factory.CreateMetricsExporter(context.Background(), params, cfg) | ||
require.Error(t, err, "expected an error when creating a traces exporter") | ||
} | ||
|
||
func TestFactory_CreateTracesExporter_Fail(t *testing.T) { | ||
factory := NewFactory() | ||
cfg := factory.CreateDefaultConfig() | ||
params := exportertest.NewNopCreateSettings() | ||
_, err := factory.CreateTracesExporter(context.Background(), params, cfg) | ||
require.Error(t, err, "expected an error when creating a traces exporter") | ||
} |
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.
Little confused here, but why is the trace exporter creation expecting error? Is it just because no endpoint? Can we add a test with trace exporter without 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.
Yes, it fails because configuration lacks endpoints and fail validation.
I'll add the success test.
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.
Looks good to me pending some readme items and small questions I had
Signed-off-by: MaxKsyunz <[email protected]>
Description: An exporter of logs and traces from OpenTelemetry to OpenSearch.
Testing: unit tests
Documentation: README.md with overview and description of configuration options.