-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(openobserve transform): Add OpenObserve as an officially supported sink #21531
base: master
Are you sure you want to change the base?
feat(openobserve transform): Add OpenObserve as an officially supported sink #21531
Conversation
@jszwedko could you help me review this? Is anything pending from my end? |
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.
Hello, this PR doesn't require any code changes and it seems more like a user guide on how to configure an existing sink. It feels like https://github.com/vectordotdev/vector-demos might be the right place for this. Or it could live under a new Community
section in https://vector.dev/guides/
d9abffa
to
e9bd1fe
Compare
* implement SinkConfig for OpenObserveConfig * chore: add comments and update reference samples * chore: add openobserve to urls and services
src/sinks/openobserve.rs
Outdated
async fn build(&self, cx: SinkContext) -> crate::Result<(VectorSink, Healthcheck)> { | ||
let request = self.request.clone(); | ||
|
||
// OpenObserve supports native HTTP ingest endpoint. This configuration wraps |
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 would move this above SinkConfig
and rephrase as:
/// This sink wraps the Vector HTTP sink to provide official support for OpenObserve's
/// native HTTP ingest endpoint. By doing so, it maintains a distinct configuration for
/// the OpenObserve sink, separate from the Vector HTTP sink. This approach ensures
/// that future changes to OpenObserve's interface can be accommodated without impacting
/// the underlying Vector HTTP sink.
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.
done
Makefile
Outdated
@@ -367,7 +367,7 @@ test-integration: ## Runs all integration tests | |||
test-integration: test-integration-amqp test-integration-appsignal test-integration-aws test-integration-axiom test-integration-azure test-integration-chronicle test-integration-clickhouse | |||
test-integration: test-integration-databend test-integration-docker-logs test-integration-elasticsearch | |||
test-integration: test-integration-eventstoredb test-integration-fluent test-integration-gcp test-integration-greptimedb test-integration-humio test-integration-http-client test-integration-influxdb | |||
test-integration: test-integration-kafka test-integration-logstash test-integration-loki test-integration-mongodb test-integration-nats | |||
test-integration: test-integration-kafka test-integration-logstash test-integration-loki test-integration-mongodb test-integration-nats test-integration-openobserve |
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 don't see any use of test-integration-openobserve
in this PR. Can we delete 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.
sure
Cargo.toml
Outdated
@@ -885,6 +888,7 @@ mqtt-integration-tests = ["sinks-mqtt"] | |||
nats-integration-tests = ["sinks-nats", "sources-nats"] | |||
nginx-integration-tests = ["sources-nginx_metrics"] | |||
opentelemetry-integration-tests = ["sources-opentelemetry", "dep:prost"] | |||
openobserve-integration-tests = ["sinks-openobserve"] |
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 don't see any use of openobserve-integration-tests
in this PR. Can we delete it here and from the github workflows?
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
.github/workflows/changes.yml
Outdated
@@ -104,6 +104,8 @@ on: | |||
value: ${{ jobs.int_tests.outputs.nginx }} | |||
opentelemetry: | |||
value: ${{ jobs.int_tests.outputs.opentelemetry }} | |||
openobserve: |
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.
int_tests
is a GitHub job that detects if a branch contains changes to integration tests. Since you didn't added any integration tests dedicated to openobserve
you should revert all changes to .github/workflows/*
,
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.
make sense
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.
👋 Left some nits. Since it affects the generated docs, you can run make generate-component-docs && make check-component-docs
to regren and validate.
Apply suggestions Co-authored-by: Pavlos Rontidis <[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.
Thank you, this looks almost ready to merge 🚀
One comment here.
removing the trailing space
I have removed the space @pront |
Head branch was pushed to by a user without write access
Please add a changelog entry. You can find details on this document. |
Head branch was pushed to by a user without write access
added @pront |
Co-authored-by: Pavlos Rontidis <[email protected]>
Head branch was pushed to by a user without write access
Let's add a new "openobserve transform" scope somewhere here: https://github.com/vectordotdev/vector/blob/master/.github/workflows/semantic.yml#L182 |
Also, please see Line 89 in ac1e975
|
This PR adds OpenObserve as an officially supported sink in Vector's documentation. OpenObserve utilizes an HTTP-based API for data ingestion, and this PR proposes updating the documentation with an example configuration for users to follow.
No new functionality is required, as the current HTTP sink already serves the purpose effectively. The primary goal is to improve user experience by offering clear guidance for setting up Vector with OpenObserve, ensuring confidence in its usage as a supported observability solution.
Why This Matters?
By officially documenting OpenObserve as a supported sink, we provide clear setup instructions for users, allowing them to confidently configure Vector for OpenObserve.
Closes: #21514