Skip to content
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

"Add 'otel' feature for OpenTelemetry support #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LikhithST
Copy link

Enabling the 'otel' feature conditionally includes OpenTelemetry and related tracing functionalities, allowing method-level tracing and context propagation."

Enabling the 'otel' feature conditionally includes OpenTelemetry and related tracing functionalities, allowing method-level tracing and context propagation."

Signed-off-by: Likhith Thammegowda <[email protected]>
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

update the branch pls

if let Some(metadata_description) = update.description {
self.metadata.description = metadata_description;
// changed.insert(Field::ActuatorTarget);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

these changes at description are not mergeable we need to find another way to propagate the context to the client/provider.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking if we could add this to the types.proto file

message MetadataMap {
  map<string, string> headers = 1;
}

message Metadata {
  optional MetadataMap otel_context = 18;  // to add this field alongside already existing fields in Metadata
}

By doing this, a new field is added to Metadata alongside description, while allowing the description to continue functioning as before ( I will revert all changes made to the description, ensuring it functions as it originally did ).

MetadataMap could be used for otel context propagation and here is the output of subscribe call with the field otel_context:

{
    "updates": [
        {
            "fields": [
                "FIELD_VALUE"
            ],
            "entry": {
                "path": "Vehicle.Speed",
                "value": {
                    "timestamp": {
                        "seconds": "1733672625",
                        "nanos": 127403578
                    },
                    "float": 22
                },
                "actuator_target": null,
                "metadata": {
                    "data_type": "DATA_TYPE_UNSPECIFIED",
                    "entry_type": "ENTRY_TYPE_UNSPECIFIED",
                    "value_restriction": null,
                    "description": "Odometer reading, total distance traveled during the lifetime of the vehicle.",
                    "unit": "km/h",
                    "otel_context": {
                        "headers": {
                            "tracestate": "",
                            "traceparent": "00-d9b7e6227856aefc7fe4ba0e142e2017-b6923a717115ffae-01"
                        }
                    }
                }
            }
        }
    ]
}

I have tested this change locally and it works as expected.

Please let me know if this approach is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

no changes in proto as of now. Could you confirm that the tracing stuff is working with the context stuff removed?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have created a new pull request #119 containing just the tracing functionality.
I have tested this change locally and it works fine.

databroker/src/broker.rs Outdated Show resolved Hide resolved
databroker/src/grpc/kuksa_val_v1/val.rs Show resolved Hide resolved
databroker/src/grpc/kuksa_val_v1/val.rs Outdated Show resolved Hide resolved
@lukasmittag
Copy link
Contributor

no documentation how to start etc. would be nice if there would be something. As well as an example of a client & provider example for context propagation.

@LikhithST
Copy link
Author

I will add readme explaining environment variables required and context propagation mechanism between the clients and the databroker in detail in this branch testing-main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants