Skip to content

Add opentelemetry to mapper-framework for enhancing pushmethod.#5628

Merged
kubeedge-bot merged 8 commits into
kubeedge:masterfrom
forsaken628:otel
Oct 23, 2024
Merged

Add opentelemetry to mapper-framework for enhancing pushmethod.#5628
kubeedge-bot merged 8 commits into
kubeedge:masterfrom
forsaken628:otel

Conversation

@forsaken628

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add opentelemetry to mapper-framework for enhancing pushmethod db.

Which issue(s) this PR fixes:

Fixes #5478

Special notes for your reviewer:
similar as #5376

Does this PR introduce a user-facing change?:

Add opentelemetry to mapper-framework for enhancing pushmethod db. 

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label May 23, 2024
@kubeedge-bot kubeedge-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 23, 2024
@Shelley-BaoYue

Copy link
Copy Markdown
Collaborator

cc @wbc6080
Could you please add more descriptive in the title of each commit?

@wbc6080 wbc6080 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you very much for your contribution! But I have a few questions about this solution:
1.opentelemetry does not seem to be a database. I don’t quite understand why this part of the function should be placed in the database processing part of the mapper-framework data plane.
2. In addition to providing the function of pushing to the database, the current mapper-framework can also push data to user applications through http/mqtt. Why can't we directly access the prometheus api through http to complete data push, but need to use opentelemetry?

@forsaken628

Copy link
Copy Markdown
Contributor Author

1.opentelemetry does not seem to be a database. I don’t quite understand why this part of the function should be placed in the database processing part of the mapper-framework data plane.

fixed

  1. In addition to providing the function of pushing to the database, the current mapper-framework can also push data to user applications through http/mqtt. Why can't we directly access the prometheus api through http to complete data push, but need to use opentelemetry?

The current http implementation does not match this remote_write_spec

The following headers MUST be sent with the HTTP request:

    Content-Encoding: snappy
    Content-Type: application/x-protobuf
    User-Agent: <name & version of the sender>
    X-Prometheus-Remote-Write-Version: 0.1.0

And why we should providing opentelemetry push?

https://opentelemetry.io/ecosystem/vendors/
https://opentelemetry.io/docs/collector/
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter

@forsaken628 forsaken628 changed the title Add opentelemetry to mapper-framework for enhancing pushmethod db. Add opentelemetry to mapper-framework for enhancing pushmethod. May 24, 2024
@Shelley-BaoYue

Copy link
Copy Markdown
Collaborator

Hi, @forsaken628 Would you be willing to attend KubeEdge community meeting (Wednesdays at 16:00-17:30 every week)? we can disscuss in detail about this feature.

@luomengY

Copy link
Copy Markdown
Member

opentelemetry

Is it possible to change the differences you mentioned based on the existing HTTP push methods? Here, the push method is for some databases, and it may not be necessary to add a separate opentelemetry push method.

@forsaken628

Copy link
Copy Markdown
Contributor Author

The main question here is: What is HTTP push? What is pushmethod?

If it's just a URL or a configuration, and all subsequent issues can be resolved through secondary development. Please consider 'mysql://localhost:3306/db?parseTime=true'; It's also a URL. Why distinguish between HTTP push, MQTT push, and DB push?

If it's just the current implementation, it's too rudimentary to do anything.
There's an abundance of HTTP-based functionalities, like MySQL over HTTP and ClickHouse over HTTP.

@luomengY

Copy link
Copy Markdown
Member

The main question here is: What is HTTP push? What is pushmethod?

If it's just a URL or a configuration, and all subsequent issues can be resolved through secondary development. Please consider 'mysql://localhost:3306/db?parseTime=true'; It's also a URL. Why distinguish between HTTP push, MQTT push, and DB push?

If it's just the current implementation, it's too rudimentary to do anything. There's an abundance of HTTP-based functionalities, like MySQL over HTTP and ClickHouse over HTTP.

The endpoint of push designed with pushmethod is usually a database, such as Redis, mysql,influxdb, HTTP, on the other hand, is a user-defined API interface for pushing to upper level applications. What you mentioned here is a typical API interface for pushing to upper level applications, so it may not be appropriate to include pushmethod. If it is only suitable for the ones you listed, due to some differences between request headers and HTTP, I think adding a new type of push method may make the Mapper framework too cumbersome, after all, the Mapper framework is just a basic template framework. Or would it be more appropriate to implement a ready-made mapper based on Promethus and place it in our rep:https://github.com/kubeedge/mappers-go? @wbc6080

@forsaken628

Copy link
Copy Markdown
Contributor Author

I also don't think it's reasonable to add a specific push mechanism solely for Prometheus. However, the OpenTelemetry protocol is not only for support Prometheus but also for support various other database, including Apache SkyWalking, ClickHouse, GreptimeDB, Elastic, and so on. https://opentelemetry.io/ecosystem/vendors/

@wbc6080

wbc6080 commented May 28, 2024

Copy link
Copy Markdown
Collaborator

This is indeed a relatively large feature implementation. I think it is more appropriate to introduce it at a regular community meeting and see the opinions of other developers. @forsaken628 Do you have time to attend the regular meeting this Wednesday, May 29, from 4:00 to 5:30 pm? And could you show some materials such as proposal or other documentation to explain this part to everyone?

@forsaken628

Copy link
Copy Markdown
Contributor Author

ok @wbc6080

@Shelley-BaoYue

Copy link
Copy Markdown
Collaborator

Hi, @forsaken628 I have added this topic in community meeting agenda https://docs.google.com/document/d/1Sr5QS_Z04uPfRbA7PrXr3aPwCRpx7EtsyHq7mp6CnHs/edit#heading=h.tzlhovcfulbj. Welcome you to take part in meeting at 16:00 today, and the meeting link is https://zoom.us/j/4167237304

@JiaweiGithub

JiaweiGithub commented May 29, 2024

Copy link
Copy Markdown
Contributor

Good job ! opentelemetry is a very popular monitoring framework currently. I think the method based on opentelemetry is very good. Others can also refer to this code to realize the integration of opentelemetry and other components.

@Shelley-BaoYue Shelley-BaoYue added this to the v1.18 milestone Jul 11, 2024
@fisherxu fisherxu closed this Jul 15, 2024
@fisherxu fisherxu reopened this Jul 15, 2024

@Shelley-BaoYue Shelley-BaoYue left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please run hack/generate-crds.sh and push the modified to fix the CI.

@fisherxu fisherxu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@forsaken628 The lint error has been fixed in #5730, you can remove the last commit

Comment thread pkg/apis/dmi/v1beta1/api.proto
Comment thread staging/src/github.com/kubeedge/mapper-framework/_template/mapper/go.mod Outdated
Signed-off-by: coldWater <forsaken628@gmail.com>
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2024
Signed-off-by: coldWater <forsaken628@gmail.com>
Signed-off-by: coldWater <forsaken628@gmail.com>
Signed-off-by: coldWater <forsaken628@gmail.com>
@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024
Signed-off-by: coldWater <forsaken628@gmail.com>
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2024

@wbc6080 wbc6080 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! cc @WillardHu @fisherxu
/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
Signed-off-by: coldWater <forsaken628@gmail.com>
@kubeedge-bot kubeedge-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 23, 2024
Signed-off-by: coldWater <forsaken628@gmail.com>
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024
@WillardHu

Copy link
Copy Markdown
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2024

@fisherxu fisherxu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/approve
/hold cancel

@kubeedge-bot kubeedge-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2024
@kubeedge-bot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2024
@kubeedge-bot kubeedge-bot merged commit 93e6aad into kubeedge:master Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Device data supports storage to prometheus

8 participants