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

Opamp client api #1481

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
23 changes: 23 additions & 0 deletions opamp-client/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
plugins {
id("otel.java-conventions")
id("com.google.protobuf") version "0.9.4"
}

description = "Client implementation of the OpAMP spec."
Expand All @@ -9,3 +10,25 @@ java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}

tasks.withType<JavaCompile>().configureEach {
with(options) {
// Suppressing warnings about the usage of deprecated methods.
// This is needed because the Protobuf plugin (com.google.protobuf) generates code that uses deprecated methods.
compilerArgs.add("-Xlint:-deprecation")
}
}

val protobufVersion = "4.28.2"

protobuf {
protoc {
artifact = "com.google.protobuf:protoc:$protobufVersion"
}
}

dependencies {
implementation("com.google.protobuf:protobuf-java:$protobufVersion")
annotationProcessor("com.google.auto.value:auto-value")
compileOnly("com.google.auto.value:auto-value-annotations")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opamp.client.internal;

import io.opentelemetry.opamp.client.internal.response.MessageData;
import opamp.proto.Opamp;

public interface OpampClient {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see some of opamp-go Client's methods here, e.g. SetAgentDescription. Just wanted to confirm this is deliberate and not accidentally missing.

Copy link
Contributor Author

@LikeTheSalad LikeTheSalad Oct 1, 2024

Choose a reason for hiding this comment

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

Yes, for context, I'm currently moving the PoC implementation created here which had only the bare minimum apis/functionality for our use-case to work. So we should keep on adding more methods in the future. The SetAgentDescription one seems to be pretty straightforward so I think we could add it right away to account for the use-case of changing the description after the client has started.

An interesting note on the client initialization method used in the PoC is that it relies on the builder pattern. So, setting the agent description before starting the client is done in the client builder, as you can see here. There are 2 main reasons for this, the first one being that it's less error-prone as users wouldn't need to remember setting the agent description before starting the client, which is something I noticed is mentioned here in the Go impl, and the second reason is that using the builder pattern to solve these issues is a common practice in Java and makes it easier to adjust/extend initialization-related processes in the future. It's also the pattern used when initializing the OTel Java SDK so I think that's a bonus in terms of consistency across Java-related projects in OTel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added SetAgentDescription.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I think you are free to deviate from opamp-go and use whatever is the most idiomatic approach for Java. Builder pattern is less common in Go world. Struct of options is typically what Go uses, e.g. the standard library http server: https://pkg.go.dev/net/http#hdr-Servers


/**
* Starts the client and begin attempts to connect to the Server. Once connection is established
* the client will attempt to maintain it by reconnecting if the connection is lost. All failed
* connection attempts will be reported via {@link Callback#onConnectFailed(OpampClient,
* Throwable)} callback.
*
* <p>This method does not wait until the connection to the Server is established and will likely
* return before the connection attempts are even made.
*
* <p>This method may be called only once.
*
* @param callback The Callback to which the Client will notify about any Server requests and
* responses.
*/
void start(Callback callback);
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to add other start-related settings later? Things like server url, agent instance id, etc that we have in opamp-go.

Copy link
Contributor Author

@LikeTheSalad LikeTheSalad Oct 1, 2024

Choose a reason for hiding this comment

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

I'm happy to discuss this more in detail, though for now, that's not the plan. It might change once we get to the part where more init params are needed though, I haven't fully analyzed all of the ones in the Go struct, but for now I can mention that the builder pattern currently takes care of things such as the instance id for example (following the note here on why we're using the builder pattern).

However, regarding the server url (or anything related to network connectivity) things get a little complicated in Java. Long story short, there are multiple tools to make HTTP requests and establish websocket connections and people have strong opinions about which ones to use, so it's a common practice to abstract those processes and create implementations that make use of one of those tools. So because of that, for anything related to network connectivity (HTTP or WebSocket) we have this abstraction that works as a placeholder and then each impl can decide how to get the values it needs (such as the server url). So for example, this is the HTTP impl that contains an instance of HttpSender where we use this sender impl that receives the HTTP url when getting created. Later, when building the OpAMP client, we pass the HttpSender impl into the builder, already configured with all the params it needs to work, such as the server url. So for now the idea is to pass an implementation that already knows where and how to send messages (and receive them too in the case of the websocket impl).

Copy link
Member

Choose a reason for hiding this comment

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

I am very interested in your approach that abstracts away the connectivity responsibility. We felt a somewhat similar need in opamp-go. The reason there was that Go's HTTP client has a gazillion options and it is a slippery slope to try to add all of them to OpAMPClient API. Instead we want to find a good way to allow passing the http client to OpAMPClient. The difficulty we have is that HTTP Client and WebSocket client have very different APIs in Go, which results in different looking OpAMPClient API's depending on what OpAMP transport you use. This clashes with our desire to abstract away the transport differences at OpAMP level as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I'm not sure if what we have might work for Go too, though I'm happy to help in case you'd like to get more details about it.


/**
* Stops the client. May be called only after {@link #start(OpampClient.Callback)}. May be called
* only once. After this call returns successfully it is guaranteed that no callbacks will be
* called. Once stopped, the client cannot be started again.
*/
void stop();

/**
* Sets the current remote config status which will be sent in the next agent to server request.
*
* @param remoteConfigStatus The new remote config status.
*/
void setRemoteConfigStatus(Opamp.RemoteConfigStatus remoteConfigStatus);

/**
* Sets the current effective config which will be sent in the next agent to server request.
*
* @param effectiveConfig The new effective config.
*/
void setEffectiveConfig(Opamp.EffectiveConfig effectiveConfig);
Copy link
Member

Choose a reason for hiding this comment

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

In opamp-go effective config is provided via a callback. I know it is not consistent with the rest, but there is a reason we went that way, see comments here https://github.com/open-telemetry/opamp-go/blob/b33ab76c401d0f5c3f179c5905409c1502176f97/client/internal/clientstate.go#L26

Happy to expand more if the comments are not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must admit I thought it was strange at first because it wasn't consistent and I also missed that comment, so thank you for pointing it out 🙏

Now that I'm aware of this, I'm curious if there could be a way to address this issue while keeping some consistency with the other values at the same time 🤔 I'll have a look, and if I can't find a way I'll add the callback option 👍

Copy link
Member

Choose a reason for hiding this comment

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

Excellent. I am happy to hear any thoughts you have about how we can make this better. I will gladly take it back to opamp-go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I found a way to address this, I also like the overall impl better thanks to the changes I made for it to work so I'm glad we had this discussion.

The approach combines a Supplier and the observer pattern to create a State type which will be used for all the AgentToServer fields.

For most of the fields, the State implementation they will use is InMemoryState, which will store its value in memory, but for the effective config state, users will have to provide their own implementation when building the client.

When it's built, the client observes changes from these states to know when to add them into the next request. That way users can tell from within their own state implementation when should the client query it and send the effective config field.

I created this dummy implementation which is used in this example to show how it could be used.

The consequence of this is that the OpampClient interface won't have any setter/updater method related to the effective config because that state isn't handled by the client, so the user should keep their provided implementation somewhere handy for them to interact with it and update it/request for it to be sent in the next request. For the values that we want to store in memory though, the client keeps the setter methods as a convenient way of setting new values into the InMemoryState implementations, like this one for example. I'm not sure if we should keep these convenience methods though because probably some users will get confused about why not all fields have setters in the client, but the alternative is to make users to provide all the state implementations and keep them all somewhere handy so that they can change them at some point, so I guess the convenience methods can be very handy to make things easier in general.

I'm open to discussing more details about it if needed and also looking forward to any feedback! Cheers.


interface Callback {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Callback or Callbacks? We use Callbacks in go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll make the changes 👍

/**
* Called when the connection is successfully established to the Server. May be called after
* {@link #start(OpampClient.Callback)} is called and every time a connection is established to
* the Server. For WebSocket clients this is called after the handshake is completed without any
* error. For HTTP clients this is called for any request if the response status is OK.
*
* @param client The relevant {@link OpampClient} instance.
*/
void onConnect(OpampClient client);

/**
* Called when the connection to the Server cannot be established. May be called after {@link
* #start(OpampClient.Callback)} is called and tries to connect to the Server. May also be
* called if the connection is lost and reconnection attempt fails.
*
* @param client The relevant {@link OpampClient} instance.
* @param throwable The exception.
*/
void onConnectFailed(OpampClient client, Throwable throwable);

/**
* Called when the Server reports an error in response to some previously sent request. Useful
* for logging purposes. The Agent should not attempt to process the error by reconnecting or
* retrying previous operations. The client handles the ErrorResponse_UNAVAILABLE case
* internally by performing retries as necessary.
*
* @param client The relevant {@link OpampClient} instance.
* @param errorResponse The error returned by the Server.
*/
void onErrorResponse(OpampClient client, Opamp.ServerErrorResponse errorResponse);

/**
* Called when the Agent receives a message that needs processing. See {@link MessageData}
* definition for the data that may be available for processing. During onMessage execution the
* {@link OpampClient} functions that change the status of the client may be called, e.g. if
* RemoteConfig is processed then {@link
* #setRemoteConfigStatus(opamp.proto.Opamp.RemoteConfigStatus)} should be called to reflect the
* processing result. These functions may also be called after onMessage returns. This is
* advisable if processing can take a long time. In that case returning quickly is preferable to
* avoid blocking the {@link OpampClient}.
*
* @param client The relevant {@link OpampClient} instance.
* @param messageData The server response data that needs processing.
*/
void onMessage(OpampClient client, MessageData messageData);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I assume more callbacks will be added later, e.g. OnOpampConnectionSettings, OnCommand, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I think those should be added as soon as we add support for those functionalities (unless we see a reason to add them beforehand with noop functionality).

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opamp.client.internal.response;

import com.google.auto.value.AutoValue;
import io.opentelemetry.opamp.client.internal.OpampClient;
import javax.annotation.Nullable;
import opamp.proto.Opamp;

/**
* Data class provided in {@link OpampClient.Callback#onMessage(OpampClient, MessageData)} with
* Server's provided status changes.
*/
@AutoValue
public abstract class MessageData {
@Nullable
public abstract Opamp.AgentRemoteConfig getRemoteConfig();

public static Builder builder() {
return new AutoValue_MessageData.Builder();
}

@AutoValue.Builder
public abstract static class Builder {
public abstract Builder setRemoteConfig(Opamp.AgentRemoteConfig remoteConfig);

public abstract MessageData build();
}
}
67 changes: 67 additions & 0 deletions opamp-client/src/main/proto/anyvalue.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright 2019, OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// This file is copied and modified from https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/common/v1/common.proto
// Modifications:
// - Removal of unneeded InstrumentationLibrary and StringKeyValue messages.
// - Change of go_package to reference a package in this repo.
// - Removal of gogoproto usage.

syntax = "proto3";

package opamp.proto;

option go_package = "github.com/open-telemetry/opamp-go/protobufs";

// AnyValue is used to represent any type of attribute value. AnyValue may contain a
// primitive value such as a string or integer or it may contain an arbitrary nested
// object containing arrays, key-value lists and primitives.
message AnyValue {
// The value is one of the listed fields. It is valid for all values to be unspecified
// in which case this AnyValue is considered to be "null".
oneof value {
string string_value = 1;
bool bool_value = 2;
int64 int_value = 3;
double double_value = 4;
ArrayValue array_value = 5;
KeyValueList kvlist_value = 6;
bytes bytes_value = 7;
}
}

// ArrayValue is a list of AnyValue messages. We need ArrayValue as a message
// since oneof in AnyValue does not allow repeated fields.
message ArrayValue {
// Array of values. The array may be empty (contain 0 elements).
repeated AnyValue values = 1;
}

// KeyValueList is a list of KeyValue messages. We need KeyValueList as a message
// since `oneof` in AnyValue does not allow repeated fields. Everywhere else where we need
// a list of KeyValue messages (e.g. in Span) we use `repeated KeyValue` directly to
// avoid unnecessary extra wrapping (which slows down the protocol). The 2 approaches
// are semantically equivalent.
message KeyValueList {
// A collection of key/value pairs of key-value pairs. The list may be empty (may
// contain 0 elements).
repeated KeyValue values = 1;
}

// KeyValue is a key-value pair that is used to store Span attributes, Link
// attributes, etc.
message KeyValue {
string key = 1;
AnyValue value = 2;
}
Loading
Loading