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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add /info URL to provide information about server application #5626

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you 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:
*
* https://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
*/

package com.linecorp.armeria.server.management;

import java.util.Objects;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.MoreObjects;

import com.linecorp.armeria.common.annotation.Nullable;

/**
* A class that represents application information, which can be configured through
* {@link ManagementService#of(AppInfo)}.
*/
public final class AppInfo {
@Nullable final String version;
@Nullable final String name;
@Nullable final String description;
Comment on lines +31 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@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) {
Comment on lines +35 to +41
Copy link
Member

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 adding of() instead, just in case we add more properties?
  • Maybe name could come before version?

this.version = version;
this.name = name;
this.description = description;
}
ikhoon marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the artifact version of the deployed application, such as {@code "1.0.0"}.
*/
@JsonProperty
public String getVersion() {
Copy link
Member

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

return version;
}

/**
* Returns the name of the deployed application.
*/
@JsonProperty
public String getName() {
return name;
}

/**
* Returns the description of the deployed application.
*/
@JsonProperty
public String getDescription() {
return description;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("version", version)
.add("name", name)
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.add("version", version)
.add("name", name)
.add("name", name)
.add("version", version)

.add("description", description)
.toString();
}
Comment on lines +71 to +78
Copy link
Member

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?


@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}

if (o == null || getClass() != o.getClass()) {
return false;
}

final AppInfo appInfo = (AppInfo) o;
return Objects.equals(version, appInfo.version) &&
Objects.equals(name, appInfo.name) &&
Objects.equals(description, appInfo.description);
}

@Override
public int hashCode() {
return Objects.hash(version, name, description);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you 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:
*
* https://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
*/

package com.linecorp.armeria.server.management;

import static java.util.Objects.requireNonNull;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.google.common.collect.ImmutableMap;

import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.MediaType;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.Version;
import com.linecorp.armeria.internal.common.JacksonUtil;
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.Server;
import com.linecorp.armeria.server.ServiceRequestContext;

/**
* An {@link HttpService} that provides additional information about configured server.
* This functionality is can be used by binding {@link ManagementService}
* It provides information about not only deployed application information, which can be specified by user,
* but also one about Armeria artifact itself.
*/
public enum AppInfoService implements HttpService {

INSTANCE;

private static final Version armeriaVersionInfo = Version.get("armeria", Server.class.getClassLoader());

@Nullable
private AggregatedHttpResponse infoAggregatedResponse;

void setAppInfo(@Nullable AppInfo appInfo) {
final byte[] data;

try {
if (appInfo == null) {
data = JacksonUtil.writeValueAsBytes(buildArmeriaInfoMap());
} else {
data = JacksonUtil.writeValueAsBytes(buildInfoMap(appInfo));
}
} catch (JsonProcessingException e) {
throw new IllegalArgumentException(e.toString(), e);
Copy link
Contributor

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

Suggested change
throw new IllegalArgumentException(e.toString(), e);
throw new IllegalArgumentException(e);

}

infoAggregatedResponse = AggregatedHttpResponse.of(HttpStatus.OK, MediaType.JSON, data);
}

@Override
public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception {
assert infoAggregatedResponse != null;
return infoAggregatedResponse.toHttpResponse();
}

private static ImmutableMap<Object, Object> buildInfoMap(AppInfo appInfo) {
requireNonNull(appInfo, "appInfo");
return ImmutableMap.builder()
.putAll(buildArmeriaInfoMap())
.putAll(buildAppInfoMap(appInfo))
.build();
}

private static ImmutableMap<String, ImmutableMap<String, String>> buildArmeriaInfoMap() {
return ImmutableMap.of(
"armeria", ImmutableMap.of(
Copy link
Contributor

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?

Suggested change
"armeria", ImmutableMap.of(
"armeria", armeriaVersionInfo)

"version", armeriaVersionInfo.artifactVersion(),
"commit", armeriaVersionInfo.longCommitHash(),
"repositoryStatus", armeriaVersionInfo.repositoryStatus()
)
);
}

private static ImmutableMap<String, ImmutableMap<String, String>> buildAppInfoMap(AppInfo appInfo) {
requireNonNull(appInfo, "appInfo");
return ImmutableMap.of(
"app", ImmutableMap.of(
Copy link
Contributor

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?

Suggested change
"app", ImmutableMap.of(
"app", appInfo)

"version", appInfo.version,
"name", appInfo.name,
"description", appInfo.description
)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.server.management;

import static java.util.Objects.requireNonNull;

import com.linecorp.armeria.common.HttpHeaderNames;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
Expand Down Expand Up @@ -64,6 +66,16 @@ public final class ManagementService extends AbstractHttpService {
* Returns a singleton {@link ManagementService}.
*/
public static ManagementService of() {
AppInfoService.INSTANCE.setAppInfo(null);
return INSTANCE;
}

/**
* Returns a singleton {@link ManagementService} with specified {@link AppInfo}.
*/
public static ManagementService of(AppInfo appInfo) {
requireNonNull(appInfo, "appInfo");
AppInfoService.INSTANCE.setAppInfo(appInfo);
Copy link
Contributor

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);
    }

return INSTANCE;
}

Expand All @@ -77,6 +89,8 @@ public HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) throws Exc
return ThreadDumpService.INSTANCE.serve(ctx, req);
case "/jvm/heapdump":
return HeapDumpService.INSTANCE.serve(ctx, req);
case "/info":
return AppInfoService.INSTANCE.serve(ctx, req);
default:
return HttpResponse.of(HttpStatus.NOT_FOUND);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;

import com.linecorp.armeria.client.WebClient;
Expand All @@ -43,13 +45,16 @@

class ManagementServiceTest {
private static final ObjectMapper mapper = new ObjectMapper();
private static final AppInfo TEST_APP_INFO = new AppInfo(
"1.0.0", "test-app-name", "test-app-description"
);

@RegisterExtension
static ServerExtension server = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
sb.requestTimeout(Duration.ofSeconds(45)); // Heap dump can take time.
sb.serviceUnder("/internal/management", ManagementService.of());
sb.serviceUnder("/internal/management", ManagementService.of(TEST_APP_INFO));
}
};

Expand Down Expand Up @@ -103,4 +108,22 @@ void heapDump() throws InterruptedException {
// Make sure that the returned file has a valid hprof format
assertThat(Arrays.copyOf(actual, fileHeader.length)).isEqualTo(fileHeader);
}

@Test
void appInfo() throws JsonProcessingException {
final WebClient client = WebClient.builder(server.httpUri())
.build();

final AggregatedHttpResponse response = client.get("/internal/management/info").aggregate().join();
final String content = response.contentUtf8();
final JsonNode jsonNode = mapper.readTree(content).path("app");
Copy link
Contributor

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

Copy link
Contributor

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?


final String actualVersion = jsonNode.path("version").textValue();
final String actualName = jsonNode.path("name").textValue();
final String actualDescription = jsonNode.path("description").textValue();

assertThat(actualVersion).isEqualTo(TEST_APP_INFO.version);
assertThat(actualName).isEqualTo(TEST_APP_INFO.name);
assertThat(actualDescription).isEqualTo(TEST_APP_INFO.description);
}
}