[NA] Add info logs #242

thiagohora · 2024-09-13T16:25:57Z

Details

Many parts of the service were lacking basic info logs. This first iteration added a few basic logs; next, we may add more debug logs and any remaining relevant info logs.

Issues

Resolves #

Testing

Documentation

LifeXplorer

Looks good. Added few points to consider

LifeXplorer · 2024-09-16T07:38:14Z

apps/opik-backend/src/main/java/com/comet/opik/domain/TraceService.java

@@ -251,10 +253,12 @@ private Mono<Project> getProjectByName(String projectName) {
    }

    private <T> Mono<T> failWithConflict(String error) {
+        log.info(error);


To avoid pollution, it may have sense to log only once by identifier (projectName) in case these requests may be frequent. Smth like we have in EM: https://github.com/comet-ml/comet-backend/blob/f287bed3cfa92ba502fdb1a5728cd2ca1e55dd9a/comet-utils/src/main/java/com/comet/utils/monitoring/ErrorReporter.java#L57

Conflicts are constraint validation. I don't expect to have them very often, but we can consider this if the number increases considerably. I would just avoid adding it now if it is not necessary yet.

LifeXplorer · 2024-09-16T07:42:22Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DatasetsResource.java

-        return Response.ok().entity(service.findById(id)).build();
+        String workspaceId = requestContext.get().getWorkspaceId();
+
+        log.info("Finding dataset by id '{}' on workspaceId '{}'", id, workspaceId);


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

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.

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

andrescrz

Just stylish comments, but no blockers.

andrescrz · 2024-09-16T09:55:51Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DatasetsResource.java

-        return Response.ok().entity(service.findById(id)).build();
+        String workspaceId = requestContext.get().getWorkspaceId();
+
+        log.info("Finding dataset by id '{}' on workspaceId '{}'", id, workspaceId);


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

andrescrz · 2024-09-16T10:00:20Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/DatasetsResource.java

+        String workspaceId = requestContext.get().getWorkspaceId();
+
+        log.info("Finding datasets by '{}' on workspaceId '{}'", criteria, workspaceId);
+        DatasetPage datasetPage = service.find(page, size, criteria);


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.

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

andrescrz · 2024-09-16T10:02:10Z

...k-backend/src/main/java/com/comet/opik/api/resources/v1/priv/FeedbackDefinitionResource.java

@@ -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) {


Functional change here, looks fine but be careful.

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

andrescrz · 2024-09-16T10:03:00Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/SpansResource.java

@@ -51,6 +51,8 @@
 import java.util.UUID;
 import java.util.stream.Collectors;

+import static com.comet.opik.api.Span.SpanPage;
+import static com.comet.opik.api.Span.View;


Minor: not a big fan of static imports in general. For this particular case, you won't know which View it is.

Good point; in the case of the view, it's a bit confusing. I will remove it.

andrescrz · 2024-09-16T10:05:04Z

apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentItemService.java

@@ -27,7 +27,7 @@ public class ExperimentItemService {
    private final @NonNull ExperimentService experimentService;
    private final @NonNull DatasetItemDAO datasetItemDAO;

-    public Mono<Void> create(Set<ExperimentItem> experimentItems) {
+    public Mono<Void> create(@NonNull Set<ExperimentItem> experimentItems) {


Minor: non-null not needed, the call below already validates it.

* add docker compose and simple nginx confi for it * Updated Opik documentation with new deployment options * [NA] Add info logs (#242) * [OPIK-91] [UX improvements] Implement the Experiment configuration section in the compare page (#252) * Update docs * Removed images * Update to self-host documentation --------- Co-authored-by: liyaka <liyak@comet.com> Co-authored-by: Andres Cruz <andresc@comet.com> Co-authored-by: Thiago dos Santos Hora <thiagoh@comet.com> Co-authored-by: andrii.dudar <andriid@comet.com>

[NA] Add info logs

de49b84

thiagohora requested a review from a team as a code owner September 13, 2024 16:25

thiagohora self-assigned this Sep 13, 2024

Merge branch 'main' into thiagohora/add_logs

712d1a3

LifeXplorer approved these changes Sep 16, 2024

View reviewed changes

andrescrz approved these changes Sep 16, 2024

View reviewed changes

thiagohora merged commit ac91bf0 into main Sep 16, 2024
2 checks passed

thiagohora deleted the thiagohora/add_logs branch September 16, 2024 10:27

jverre pushed a commit that referenced this pull request Sep 16, 2024

[NA] Add info logs (#242)

98698f0

dsblank pushed a commit that referenced this pull request Oct 4, 2024

[NA] Add info logs (#242)

2d718e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NA] Add info logs #242

[NA] Add info logs #242

thiagohora commented Sep 13, 2024 •

edited

Loading

LifeXplorer left a comment

LifeXplorer Sep 16, 2024

thiagohora Sep 16, 2024

LifeXplorer Sep 16, 2024

thiagohora Sep 16, 2024

andrescrz Sep 16, 2024

andrescrz left a comment

andrescrz Sep 16, 2024

andrescrz Sep 16, 2024

thiagohora Sep 16, 2024

andrescrz Sep 16, 2024

thiagohora Sep 16, 2024

andrescrz Sep 16, 2024

thiagohora Sep 16, 2024

andrescrz Sep 16, 2024

[NA] Add info logs #242

[NA] Add info logs #242

Conversation

thiagohora commented Sep 13, 2024 • edited Loading

Details

Issues

Testing

Documentation

LifeXplorer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrescrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagohora commented Sep 13, 2024 •

edited

Loading