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.

Sign up for GitHub

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

Jump to bottom

[NA] Add info logs #242

Merged
merged 2 commits into from
Sep 16, 2024
Merged

[NA] Add info logs #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de49b84
[NA] Add info logs
thiagohora Sep 13, 2024
712d1a3
Merge branch 'main' into thiagohora/add_logs
thiagohora Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 94 additions & 24 deletions apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DatasetsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,13 @@ public class DatasetsResource {
@JsonView(Dataset.View.Public.class)
public Response getDatasetById(@PathParam("id") UUID id) {

return Response.ok().entity(service.findById(id)).build();
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Finding dataset by id '{}' on workspaceId '{}'", id, workspaceId);
Copy link

Choose a reason for hiding this comment

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

A general Q: what is the default log level of the app? If we plan to log INFO level by default, do we have aproxiamate expectation of the logs volume that may be generated. If the volume is too big, can make sense to consider log.debug instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @LifeXplorer,

Yeah, the default is INFO; for now, we only add logs at the API level and for expected errors. I believe this is the minimum to allow us to troubleshoot in case of errors or unexpected behaviors. We are talking about 3 or 4 logs per request, which seems reasonable. We may add more debug-level logs soon, but for information, I believe those are relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are legitimate INFO level logs in order to have insights on the normal application flow for debugging and operational purposes.

Dataset dataset = service.findById(id);
log.info("Found dataset by id '{}' on workspaceId '{}'", id, workspaceId);

return Response.ok().entity(dataset).build();
}

@GET
Expand All @@ -115,7 +121,13 @@ public Response findDatasets(
.name(name)
.build();

return Response.ok(service.find(page, size, criteria)).build();
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Finding datasets by '{}' on workspaceId '{}'", criteria, workspaceId);
DatasetPage datasetPage = service.find(page, size, criteria);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future reference, just be careful about not leaking sensitive data to the logs in objects like this criteria. In this particular case, it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I thought about it, but I assumed those were not sensitive

log.info("Found datasets by '{}', count '{}' on workspaceId '{}'", criteria, datasetPage.size(), workspaceId);

return Response.ok(datasetPage).build();
}

@POST
Expand All @@ -128,7 +140,14 @@ public Response createDataset(
@RequestBody(content = @Content(schema = @Schema(implementation = Dataset.class))) @JsonView(Dataset.View.Write.class) @NotNull @Valid Dataset dataset,
@Context UriInfo uriInfo) {

URI uri = uriInfo.getAbsolutePathBuilder().path("/%s".formatted(service.save(dataset).id().toString())).build();
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Creating dataset with name '{}', on workspace_id '{}'", dataset.name(), workspaceId);
Dataset savedDataset = service.save(dataset);
log.info("Created dataset with name '{}', id '{}', on workspace_id '{}'", savedDataset.name(),
savedDataset.id(), workspaceId);

URI uri = uriInfo.getAbsolutePathBuilder().path("/%s".formatted(savedDataset.id().toString())).build();
return Response.created(uri).build();
}

Expand All @@ -140,7 +159,11 @@ public Response createDataset(
public Response updateDataset(@PathParam("id") UUID id,
@RequestBody(content = @Content(schema = @Schema(implementation = DatasetUpdate.class))) @NotNull @Valid DatasetUpdate datasetUpdate) {

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Updating dataset by id '{}' on workspace_id '{}'", id, workspaceId);
service.update(id, datasetUpdate);
log.info("Updated dataset by id '{}' on workspace_id '{}'", id, workspaceId);

return Response.noContent().build();
}

Expand All @@ -151,7 +174,10 @@ public Response updateDataset(@PathParam("id") UUID id,
})
public Response deleteDataset(@PathParam("id") UUID id) {

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Deleting dataset by id '{}' on workspace_id '{}'", id, workspaceId);
service.delete(id);
log.info("Deleted dataset by id '{}' on workspace_id '{}'", id, workspaceId);
return Response.noContent().build();
}

Expand All @@ -163,7 +189,12 @@ public Response deleteDataset(@PathParam("id") UUID id) {
public Response deleteDatasetByName(
@RequestBody(content = @Content(schema = @Schema(implementation = DatasetIdentifier.class))) @NotNull @Valid DatasetIdentifier identifier) {

String workspaceId = requestContext.get().getWorkspaceId();

log.info("Deleting dataset by name '{}' on workspace_id '{}'", identifier.datasetName(), workspaceId);
service.delete(identifier);
log.info("Deleted dataset by name '{}' on workspace_id '{}'", identifier.datasetName(), workspaceId);

return Response.noContent().build();
}

Expand All @@ -179,7 +210,11 @@ public Response getDatasetByIdentifier(
String workspaceId = requestContext.get().getWorkspaceId();
String name = identifier.datasetName();

return Response.ok(service.findByName(workspaceId, name)).build();
log.info("Finding dataset by name '{}' on workspace_id '{}'", name, workspaceId);
Dataset dataset = service.findByName(workspaceId, name);
log.info("Found dataset by name '{}', id '{}' on workspace_id '{}'", name, dataset.id(), workspaceId);

return Response.ok(dataset).build();
}

// Dataset Item Resources
Expand All @@ -192,9 +227,15 @@ public Response getDatasetByIdentifier(
@JsonView(DatasetItem.View.Public.class)
public Response getDatasetItemById(@PathParam("itemId") @NotNull UUID itemId) {

return Response.ok(itemService.get(itemId)
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Finding dataset item by id '{}' on workspace_id '{}'", itemId, workspaceId);
DatasetItem datasetItem = itemService.get(itemId)
.contextWrite(ctx -> setRequestContext(ctx, requestContext))
.block()).build();
.block();
log.info("Found dataset item by id '{}' on workspace_id '{}'", itemId, workspaceId);

return Response.ok(datasetItem).build();
}

@GET
Expand All @@ -208,10 +249,16 @@ public Response getDatasetItems(
@QueryParam("page") @Min(1) @DefaultValue("1") int page,
@QueryParam("size") @Min(1) @DefaultValue("10") int size) {

return Response.ok(itemService.getItems(id, page, size)
String workspaceId = requestContext.get().getWorkspaceId();
log.info("Finding dataset items by id '{}', page '{}', size '{} on workspace_id '{}''", id, page, size,
workspaceId);
DatasetItem.DatasetItemPage datasetItemPage = itemService.getItems(id, page, size)
.contextWrite(ctx -> setRequestContext(ctx, requestContext))
.block())
.build();
.block();
log.info("Found dataset items by id '{}', count '{}', page '{}', size '{} on workspace_id '{}''", id,
datasetItemPage.content().size(), page, size, workspaceId);

return Response.ok(datasetItemPage).build();
}

@POST
Expand All @@ -226,29 +273,36 @@ public Response getDatasetItems(
public ChunkedOutput<JsonNode> streamDatasetItems(
@RequestBody(content = @Content(schema = @Schema(implementation = DatasetItemStreamRequest.class))) @NotNull @Valid DatasetItemStreamRequest request) {

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Streaming dataset items by '{}' on workspaceId '{}'", request, workspaceId);
return getOutputStream(request, request.steamLimit());
}

private ChunkedOutput<JsonNode> getOutputStream(DatasetItemStreamRequest request, int limit) {

final ChunkedOutput<JsonNode> outputStream = new ChunkedOutput<>(JsonNode.class, "\r\n");
ChunkedOutput<JsonNode> outputStream = new ChunkedOutput<>(JsonNode.class, "\r\n");

String workspaceId = requestContext.get().getWorkspaceId();
String userName = requestContext.get().getUserName();
String workspaceName = requestContext.get().getWorkspaceName();

Schedulers
.boundedElastic()
.schedule(() -> Mono.fromCallable(() -> service.findByName(workspaceId, request.datasetName()))
.subscribeOn(Schedulers.boundedElastic())
.flatMapMany(dataset -> itemService.getItems(dataset.id(), limit, request.lastRetrievedId()))
.doOnNext(item -> sendDatasetItems(item, outputStream))
.onErrorResume(ex -> errorHandling(ex, outputStream))
.doFinally(signalType -> closeOutput(outputStream))
.contextWrite(ctx -> ctx.put(RequestContext.USER_NAME, userName)
.put(RequestContext.WORKSPACE_NAME, workspaceName)
.put(RequestContext.WORKSPACE_ID, workspaceId))
.subscribe());
.schedule(() -> {
Mono.fromCallable(() -> service.findByName(workspaceId, request.datasetName()))
.subscribeOn(Schedulers.boundedElastic())
.flatMapMany(
dataset -> itemService.getItems(dataset.id(), limit, request.lastRetrievedId()))
.doOnNext(item -> sendDatasetItems(item, outputStream))
.onErrorResume(ex -> errorHandling(ex, outputStream))
.doFinally(signalType -> closeOutput(outputStream))
.contextWrite(ctx -> ctx.put(RequestContext.USER_NAME, userName)
.put(RequestContext.WORKSPACE_NAME, workspaceName)
.put(RequestContext.WORKSPACE_ID, workspaceId))
.subscribe();

log.info("Streamed dataset items by '{}' on workspaceId '{}'", request, workspaceId);
});

return outputStream;
}
Expand Down Expand Up @@ -304,10 +358,16 @@ public Response createDatasetItems(
return item;
}).toList();

String workspaceId = requestContext.get().getWorkspaceId();

log.info("Creating dataset items batch by datasetId '{}', datasetName '{}', size '{}' on workspaceId '{}'",
batch.datasetId(), batch.datasetId(), batch.items().size(), workspaceId);
itemService.save(new DatasetItemBatch(batch.datasetName(), batch.datasetId(), items))
.contextWrite(ctx -> setRequestContext(ctx, requestContext))
.retryWhen(AsyncUtils.handleConnectionError())
.block();
log.info("Created dataset items batch by datasetId '{}', datasetName '{}', size '{}' on workspaceId '{}'",
batch.datasetId(), batch.datasetId(), batch.items().size(), workspaceId);

return Response.noContent().build();
}
Expand All @@ -320,9 +380,14 @@ public Response createDatasetItems(
public Response deleteDatasetItems(
@RequestBody(content = @Content(schema = @Schema(implementation = DatasetItemsDelete.class))) @NotNull @Valid DatasetItemsDelete request) {

String workspaceId = requestContext.get().getWorkspaceId();

log.info("Deleting dataset items by size'{}' on workspaceId '{}'", request, workspaceId);
itemService.delete(request.itemIds())
.contextWrite(ctx -> setRequestContext(ctx, requestContext))
.block();
log.info("Deleted dataset items by size'{}' on workspaceId '{}'", request, workspaceId);

return Response.noContent().build();
}

Expand All @@ -347,13 +412,18 @@ public Response findDatasetItemsWithExperimentItems(
.entityType(FeedbackScoreDAO.EntityType.TRACE)
.build();

log.info("Finding dataset items with experiment items by '{}', page '{}', size '{}'",
datasetItemSearchCriteria, page, size);
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Finding dataset items with experiment items by '{}', page '{}', size '{}' on workspaceId '{}'",
datasetItemSearchCriteria, page, size, workspaceId);

var datasetItemPage = itemService.getItems(page, size, datasetItemSearchCriteria)
.contextWrite(ctx -> setRequestContext(ctx, requestContext))
.block();
log.info("Found dataset items with experiment items by '{}', count '{}', page '{}', size '{}'",
datasetItemSearchCriteria, datasetItemPage.content().size(), page, size);

log.info(
"Found dataset items with experiment items by '{}', count '{}', page '{}', size '{}' on workspaceId '{}'",
datasetItemSearchCriteria, datasetItemPage.content().size(), page, size, workspaceId);
return Response.ok(datasetItemPage).build();
}

Expand Down
44 changes: 35 additions & 9 deletions ...ackend/src/main/java/com/comet/opik/api/resources/v1/priv/FeedbackDefinitionResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import com.codahale.metrics.annotation.Timed;
import com.comet.opik.api.FeedbackDefinition;
import com.comet.opik.api.FeedbackDefinitionCriteria;
import com.comet.opik.api.Page;
import com.comet.opik.domain.FeedbackDefinitionService;
import com.comet.opik.infrastructure.auth.RequestContext;
import com.fasterxml.jackson.annotation.JsonView;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.headers.Header;
Expand All @@ -13,6 +15,7 @@
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.inject.Inject;
import jakarta.inject.Provider;
import jakarta.validation.Valid;
import jakarta.validation.constraints.Min;
import jakarta.validation.constraints.NotNull;
Expand Down Expand Up @@ -48,6 +51,7 @@
public class FeedbackDefinitionResource {

private final @NonNull FeedbackDefinitionService service;
private final @NonNull Provider<RequestContext> requestContext;

@GET
@Operation(operationId = "findFeedbackDefinitions", summary = "Find Feedback definitions", description = "Find Feedback definitions", responses = {
Expand All @@ -65,8 +69,13 @@ public Response find(
.type(type)
.build();

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Find feedback definitions by '{}' on workspaceId '{}'", criteria, workspaceId);
Page<FeedbackDefinition<?>> definitionPage = service.find(page, size, criteria);
log.info("Found feedback definitions by '{}' on workspaceId '{}'", criteria, workspaceId);

return Response.ok()
.entity(service.find(page, size, criteria))
.entity(definitionPage)
.build();
}

Expand All @@ -77,7 +86,13 @@ public Response find(
})
@JsonView({FeedbackDefinition.View.Public.class})
public Response getById(@PathParam("id") @NotNull UUID id) {
return Response.ok().entity(service.get(id)).build();

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Get feedback definition by id '{}' on workspaceId '{}'", id, workspaceId);
FeedbackDefinition<?> feedbackDefinition = service.get(id);
log.info("Got feedback definition by id '{}' on workspaceId '{}'", id, workspaceId);

return Response.ok().entity(feedbackDefinition).build();
}

@POST
Expand All @@ -90,8 +105,14 @@ public Response create(
FeedbackDefinition.View.Create.class}) @NotNull @Valid FeedbackDefinition<?> feedbackDefinition,
@Context UriInfo uriInfo) {

final var createdFeedbackDefinitions = service.create(feedbackDefinition);
final var uri = uriInfo.getAbsolutePathBuilder().path("/%s".formatted(createdFeedbackDefinitions.getId()))
String workspaceId = requestContext.get().getWorkspaceId();
log.info("Creating feedback definition with id '{}', name '{}' on workspaceId '{}'", feedbackDefinition.getId(),
feedbackDefinition.getName(), workspaceId);
FeedbackDefinition<?> createdFeedbackDefinition = service.create(feedbackDefinition);
log.info("Created feedback definition with id '{}', name '{}' on workspaceId '{}'",
createdFeedbackDefinition.getId(), createdFeedbackDefinition.getName(), workspaceId);

var uri = uriInfo.getAbsolutePathBuilder().path("/%s".formatted(createdFeedbackDefinition.getId()))
.build();

return Response.created(uri).build();
Expand All @@ -106,7 +127,13 @@ public Response update(final @PathParam("id") UUID id,
@RequestBody(content = @Content(schema = @Schema(implementation = FeedbackDefinition.class))) @JsonView({
FeedbackDefinition.View.Update.class}) @NotNull @Valid FeedbackDefinition<?> feedbackDefinition) {

String workspaceId = requestContext.get().getWorkspaceId();
log.info("Updating feedback definition with id '{}' on workspaceId '{}'", feedbackDefinition.getId(),
workspaceId);
service.update(id, feedbackDefinition);
log.info("Updated feedback definition with id '{}' on workspaceId '{}'", feedbackDefinition.getId(),
workspaceId);

return Response.noContent().build();
}

Expand All @@ -117,13 +144,12 @@ public Response update(final @PathParam("id") UUID id,
})
public Response deleteById(@PathParam("id") UUID id) {

var workspace = service.getWorkspaceId(id);

if (workspace == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Functional change here, looks fine but be careful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a leftover from when we changed to filter instead of controller level auth.

return Response.noContent().build();
}
String workspaceId = requestContext.get().getWorkspaceId();

log.info("Deleting feedback definition by id '{}' on workspaceId '{}'", id, workspaceId);
service.delete(id);
log.info("Deleted feedback definition by id '{}' on workspaceId '{}'", id, workspaceId);

return Response.noContent().build();
}

Expand Down
Loading