loki4j / loki-logback-appender

Fast and lightweight implementation of Logback appender for Grafana Loki
https://loki4j.github.io/loki-logback-appender/
BSD 2-Clause "Simplified" License
300 stars 26 forks source link

Repackaged classes (grpc for example) still imports the original classes #127

Closed zalavaari closed 2 years ago

zalavaari commented 2 years ago

It's easier to show an example. Here is the beginning of the re-packaged Logproto class:

package com.github.loki4j.pkg.loki.protobuf;

import com.github.loki4j.pkg.google.protobuf.GoGoProtos;
import com.github.loki4j.pkg.google.protobuf.Timestamp;
import com.github.loki4j.pkg.google.protobuf.TimestampOrBuilder;
import com.github.loki4j.pkg.google.protobuf.TimestampProto;
import com.google.protobuf.AbstractParser;
import com.google.protobuf.ByteString;
import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;

The problem here is that these classes still reference to the original GRPC classes (which might or might not present on the classpath). In our case we have these classes, but we still use GRPC 3.17.3 while loki-logback-appender uses 3.18.1, so we have exceptions due to missing methods.

Here is an example:

Exception in thread "loki4j-encoder-0" java.lang.NoSuchMethodError: 'boolean com.google.protobuf.GeneratedMessageV3.isStringEmpty(java.lang.Object)'
    at com.github.loki4j.pkg.loki.protobuf.Logproto$StreamAdapter.getSerializedSize(Logproto.java:7435)
    at com.google.protobuf@3.17.3/com.google.protobuf.CodedOutputStream$UnsafeDirectNioEncoder.writeMessageNoTag(CodedOutputStream.java:2001)
    at com.google.protobuf@3.17.3/com.google.protobuf.CodedOutputStream$UnsafeDirectNioEncoder.writeMessage(CodedOutputStream.java:1974)
    at com.github.loki4j.pkg.loki.protobuf.Logproto$PushRequest.writeTo(Logproto.java:299)
    at com.github.loki4j.client.writer.ProtobufWriter.endStreams(ProtobufWriter.java:85)
    at com.github.loki4j.client.writer.ProtobufWriter.serializeBatch(ProtobufWriter.java:45)
    at com.github.loki4j.client.pipeline.DefaultPipeline.writeBatch(DefaultPipeline.java:227)
    at com.github.loki4j.client.pipeline.DefaultPipeline.encodeStep(DefaultPipeline.java:208)
    at com.github.loki4j.client.pipeline.DefaultPipeline.runEncodeLoop(DefaultPipeline.java:173)
    at com.github.loki4j.client.pipeline.DefaultPipeline.lambda$start$3(DefaultPipeline.java:111)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:829)

Thank you for your work!

nehaev commented 2 years ago

Hi @zalavaari! Thanks for you report again!

Yeah, given that protobuf versions 3.x are incompatible with each other, it's really hard to use loki4j in protobuf mode if there are other protobuf dependencies. I wonder how other java libs that provide protobuf-generated classes deal with this.

The obvious solution is to repackage entire protobuf runtime under com.github.loki4j.pkg. But I would like to avoid this as protobuf support is optional for loki4j.

Another solution is to extract protobuf-generated classes into a separate jar and provide several versions of this jar compiled for the different versions of protobuf (e.g. 3.15.x, 3.16.x, etc.) And even if there would be no pre-compiled jar for a particular version of protobuf, these who really need it could generate Logproto classes on their own and include them into the classpath instead of this jar without any conflicts. This also won't affect these who use loki4j in json mode. Currently I tend to explore this alternative.

If anyone else has any other suggestions feel free to put them in comments for this issue.

zalavaari commented 2 years ago

Hm. I like the idea of the separated grpc jars. I guess the result would be also a smaller, grpc-free loki-logback-appender base jar for those who using json only. The question is the maintenance cost of the added complexity. I don't have Gradle experiences, but based on Maven I guess it would be feasible.