wu-sheng commented on a change in pull request #6243:
URL: https://github.com/apache/skywalking/pull/6243#discussion_r561856410
##########
File path: docs/en/setup/backend/backend-receivers.md
##########
@@ -182,7 +182,9 @@ receiver-otel:
enabledHandlers: ${SW_OTEL_RECEIVER_ENABLED_HANDLERS:"oc"}
enabledOcRules: ${SW_OTEL_RECEIVER_ENABLED_OC_RULES:"istio-controlplane"}
```
-
+The receiver will add a labelkey 'node_identifier_host_name' to the collected
data samples，
+the labelvalue is from
opencensus.proto.agent.common.v1.Node.identifier.host_name,
+to support the identification of the source of the metric data.
Review comment:
```suggestion
The receiver adds a label with `key=node_identifier_host_name` to the
collected data samples，
and value from `Node.identifier.host_name` defined in opencensus agent
proto,
to be the identification of the metric data.
```
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -107,6 +111,15 @@
return result;
}
+ private static Map<String, String> buildLabelsWithNodeInfo(Node node,
Review comment:
```suggestion
private static Map<String, String> buildLabelsFromNodeInfo(Node node,
```
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -107,6 +111,15 @@
return result;
}
+ private static Map<String, String> buildLabelsWithNodeInfo(Node node,
+ List<LabelKey>
keys,
+
List<LabelValue> values) {
+ Map<String, String> result = buildLabels(keys, values);
Review comment:
Take a look at this comment in the original proto
```
// This is required only in the first message on the stream or if the
// previous sent ExportMetricsServiceRequest message has a different Node
(e.g.
// when the same RPC is used to send Metrics from multiple Applications).
opencensus.proto.agent.common.v1.Node node = 1;
```
You need to hold the previous node, if the current `request#hasNode()` is
false, use the previous one.
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -107,6 +111,15 @@
return result;
}
+ private static Map<String, String> buildLabelsWithNodeInfo(Node node,
Review comment:
Please don't hide `buildLabels` in the `buildLabelsWithNodeInfo`, it is
misleading the reader. Do like this `buildLabelsFromNodeInfo(buildLables(...))`.
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -57,11 +58,21 @@
@Override public StreamObserver<ExportMetricsServiceRequest> export(
StreamObserver<ExportMetricsServiceResponse> responseObserver) {
return new StreamObserver<ExportMetricsServiceRequest>() {
- @Override public void onNext(ExportMetricsServiceRequest request) {
+ private Node node;
+
+ @Override
+ public void onNext(ExportMetricsServiceRequest request) {
+ if (request.hasNode())
+ node = request.getNode();
Review comment:
I am not sure whether without `{}` is allowed. We will see.
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -107,6 +118,13 @@
return result;
}
+ private static Map<String, String> buildLabelsFromNodeInfo(Node node,
+ Map<String,
String> buildLabelsResult) {
+ if (node != null)
+ buildLabelsResult.put("node_identifier_host_name",
node.getIdentifier().getHostName());
Review comment:
```
// Identifier that uniquely identifies a process within a VM/container.
message ProcessIdentifier {
// The host name. Usually refers to the machine/container name.
// For example: os.Hostname() in Go, socket.gethostname() in Python.
string host_name = 1;
// Process id.
uint32 pid = 2;
// Start time of this ProcessIdentifier. Represented in epoch time.
google.protobuf.Timestamp start_timestamp = 3;
}
```
I think there is only `pid` makes sense? Do you think `_` is good, or should
consider `.`? Such as `node.identifier.host_name`.
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -57,11 +58,21 @@
@Override public StreamObserver<ExportMetricsServiceRequest> export(
StreamObserver<ExportMetricsServiceResponse> responseObserver) {
return new StreamObserver<ExportMetricsServiceRequest>() {
- @Override public void onNext(ExportMetricsServiceRequest request) {
+ private Node node;
+
+ @Override
+ public void onNext(ExportMetricsServiceRequest request) {
+ if (request.hasNode())
+ node = request.getNode();
Review comment:
Do you mean build an node with mock values of fields we need?
##########
File path:
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/oc/OCMetricHandler.java
##########
@@ -107,6 +118,13 @@
return result;
}
+ private static Map<String, String> buildLabelsFromNodeInfo(Node node,
+ Map<String,
String> buildLabelsResult) {
+ if (node != null)
+ buildLabelsResult.put("node_identifier_host_name",
node.getIdentifier().getHostName());
Review comment:
OK, then `_` is fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
