-
Notifications
You must be signed in to change notification settings - Fork 900
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 /info URL to provide information about server application #5626
base: main
Are you sure you want to change the base?
Conversation
@@ -151,7 +155,8 @@ final class DefaultServerConfig implements ServerConfig { | |||
DependencyInjector dependencyInjector, | |||
Function<? super String, String> absoluteUriTransformer, | |||
long unhandledExceptionsReportIntervalMillis, | |||
List<ShutdownSupport> shutdownSupports) { | |||
List<ShutdownSupport> shutdownSupports, | |||
AppInfo appInfo) { |
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.
AppInfo appInfo) { | |
@Nullable AppInfo appInfo) { |
* A class that represents application information, which can be configured through | ||
* {@link ServerBuilder#setAppInfo(AppInfo)}. | ||
*/ | ||
public class AppInfo { |
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.
public class AppInfo { | |
public final class AppInfo { |
* @param name A name of an application | ||
* @param description A description of application | ||
*/ | ||
public AppInfo(String version, String name, String description) { |
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.
public AppInfo(String version, String name, String description) { | |
public AppInfo(@Nullable String version, @Nullable String name, @Nullable String description) { |
"app", ImmutableMap.of( | ||
"version", appInfo.version, | ||
"name", appInfo.name, | ||
"description", appInfo.description |
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.
Should we annotate getters of AppInfo
with @JsonProperty
to directly serialize the value? For example:
armeria/core/src/main/java/com/linecorp/armeria/common/util/Version.java
Lines 263 to 264 in e1d1377
@JsonProperty | |
public String longCommitHash() { |
final ImmutableMap<Object, Object> infoMap = ImmutableMap.builder() | ||
.putAll(appInfoMap) | ||
.putAll(armeriaInfoMap) | ||
.build(); |
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.
We don't have to serialize the same value for each request.
Should we cache the serialized value in the member field?
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.
Could you guide a bit more about this comment?
What would be the appropriate way to cache the value of serialization result 🤔?
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.
public enum AppInfoService implements HttpService {
@Nullablle
private HttpResponse res; // here!
@Override
public HttpResponse serve(..) {
if (res != null) {
return res;
}
..
res = HttpResponse.ofJson(infoMap);
return res;
}
Maybe simply like this?
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.
Unfortunately, we can't reuse HttpResponse
after subscribing to it.
Instead, we can use AggregatedHttpResponse
. For example:
private static final Version versionInfo = Version.get("armeria", Server.class.getClassLoader());
@Null
private AggregatedHttpResponse aggregatedResponse;
void setAppInfo(@Nullable AppInfo appInfo) {
byte[] data;
if (appInfo == null) {
JacksonUtil.writeValueAsBytes(...);
} else {
...
}
aggregatedResponse = AggregatedHttpResponse.of(HttpStatus.OK, MediaType.JSON, data);
}
public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception {
assert aggregatedResponse != null;
return aggregatedResponse.toHttpResponse();
}
* } | ||
* </pre> | ||
*/ | ||
public ServerBuilder setAppInfo(AppInfo appInfo) { |
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.
Should we also expose AppInfo
to Armeria's documentation service?
Application information
table could be added if AppInfo
exists.
armeria/docs-client/src/containers/HomePage/index.tsx
Lines 95 to 99 in e1d1377
{tableRows.length > 0 && ( | |
<> | |
<Typography variant="subtitle1" paragraph> | |
Version information | |
</Typography> |
/** | ||
* Returns the {@link AppInfo} that represents application information. | ||
*/ | ||
@Nullable |
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.
Global comment: Let's add @UnstableApi
to all new public APIs.
@@ -315,6 +316,11 @@ public long unhandledExceptionsReportIntervalMillis() { | |||
return delegate.unhandledExceptionsReportIntervalMillis(); | |||
} | |||
|
|||
@Override |
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.
@Override | |
@Nullable | |
@Override |
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 think we need to decide whether we want to add AppInfo
property to ServerConfig
or not. At the moment, its usage is fairly limited and has no use for users who doesn't want to use ManagementService
. That being said, I'd suggest moving the appInfo property to ManagementService
and let users specify it when constructing a ManagementService
, not when constructing a Server
.
When I performed a rolling update or a canary deployment, a metric exporting the current system's version was useful to check the release process. We only expose Armeria version metrics. I think it would be useful to expose the application version in addition to the framework version.
|
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.
So far so good! Left some minor comments regarding naming and code layout.
* Returns the artifact version of the deployed application, such as {@code "1.0.0"}. | ||
*/ | ||
@JsonProperty | ||
public String getVersion() { |
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.
- Could you remove the
get
prefixes from all getter methods for consistency with our API? - Missing
@Nullable
annotation for all getter methods
@Override | ||
public String toString() { | ||
return MoreObjects.toStringHelper(this) | ||
.add("version", version) | ||
.add("name", name) | ||
.add("description", description) | ||
.toString(); | ||
} |
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.
Could you move this method to the bottom of the class definition, for consistency with our existing code?
@Nullable final String version; | ||
@Nullable final String name; | ||
@Nullable final String description; |
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.
@Nullable final String version; | |
@Nullable final String name; | |
@Nullable final String description; | |
@Nullable | |
private final String name; | |
@Nullable | |
private final String version; | |
@Nullable | |
private final String description; |
/** | ||
* Creates a new {@link AppInfo} that holds information about an application. | ||
* @param version A version of an application e.g. "1.0.0" | ||
* @param name A name of an application | ||
* @param description A description of application | ||
*/ | ||
public AppInfo(@Nullable String version, @Nullable String name, @Nullable String description) { |
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.
- What do you think about making this constructor
private
and addingof()
instead, just in case we add more properties? - Maybe
name
could come beforeversion
?
.add("version", version) | ||
.add("name", name) |
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.
.add("version", version) | |
.add("name", name) | |
.add("name", name) | |
.add("version", version) |
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.
Looks mostly good 👍 Left some comments
|
||
final AggregatedHttpResponse response = client.get("/internal/management/info").aggregate().join(); | ||
final String content = response.contentUtf8(); | ||
final JsonNode jsonNode = mapper.readTree(content).path("app"); |
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.
Optional) We could use assertThatJson
for fluent validations for json objects
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.
Can you also validate that the armeria
node is also serialized/deserialized correctly?
|
||
private static ImmutableMap<String, ImmutableMap<String, String>> buildArmeriaInfoMap() { | ||
return ImmutableMap.of( | ||
"armeria", ImmutableMap.of( |
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.
Question) Couldn't we just return the entire version?
"armeria", ImmutableMap.of( | |
"armeria", armeriaVersionInfo) |
private static ImmutableMap<String, ImmutableMap<String, String>> buildAppInfoMap(AppInfo appInfo) { | ||
requireNonNull(appInfo, "appInfo"); | ||
return ImmutableMap.of( | ||
"app", ImmutableMap.of( |
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.
ditto, can't we just serialize the entire AppInfo
?
"app", ImmutableMap.of( | |
"app", appInfo) |
data = JacksonUtil.writeValueAsBytes(buildInfoMap(appInfo)); | ||
} | ||
} catch (JsonProcessingException e) { | ||
throw new IllegalArgumentException(e.toString(), e); |
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.
There's probably no point in doing toString
as the cause exception is already included
throw new IllegalArgumentException(e.toString(), e); | |
throw new IllegalArgumentException(e); |
*/ | ||
public static ManagementService of(AppInfo appInfo) { | ||
requireNonNull(appInfo, "appInfo"); | ||
AppInfoService.INSTANCE.setAppInfo(appInfo); |
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.
This can be overwritten if there are multiple servers in the same JVM.
Although this can give a slight advantage of not allocating an object, I don't think ManagementService
will be allocated often anyways.
What do you think of just allocating a new ManagementService
?
e.g.
public static ManagementService of() {
return INSTANCE;
}
public static ManagementService of(AppInfo appInfo) {
requireNonNull(appInfo, "appInfo");
return new ManagementService(appInfo);
}
Motivation:
Please refer #5605 for motivation.
If it's required to provide more information about this PR, please let me know, then I'll supplement it 🙇
Modifications:
AppInfo
class that represents additional information about deployed application.AppInfoService
as one ofHttpService
that serves information about deployed application itself, as well as one about Armeria artifact itself./info
forManagementService
so that requests is routed toAppInfoService
AppInfo
as one of property ofServerConfig
setAppInfo
API inServerBuilder
so user can specifyAppInfo
when they configure Armeria serverResult:
ManagementService