-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Merge Zipkin v3 to the main branch. #3594
Conversation
* Support query dependency
@@ -79,7 +79,7 @@ LABEL org.opencontainers.image.description="Zipkin slim distribution on OpenJDK | |||
|
|||
COPY --from=install --chown=${USER} /install/zipkin-slim/ /zipkin/ | |||
|
|||
EXPOSE 9411 | |||
EXPOSE 9411 9412 |
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.
Curious about 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.
I will delete it.
@@ -40,4 +41,4 @@ if [ "$commit_local_release_branch" != "$commit_remote_release_branch" ]; then | |||
fi | |||
|
|||
# Prepare and push release commits and the version tag (N.N.N), which triggers deployment. | |||
./mvnw --batch-mode -nsu -DreleaseVersion=${release_version} -Denforcer.fail=false -Darguments="-DskipTests -Denforcer.fail=false" release:prepare | |||
./mvnw --batch-mode -nsu -DreleaseVersion=${release_version} -Denforcer.fail=false -Darguments="-DskipTests -Denforcer.fail=false -Dcheckstyle.skip=true" release:prepare |
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.
Curious about this, are we skipping the check style?
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.
At present, zipkin does not include checkstyle. However, the newly introduced skywalking will search for and utilize the checkstyle configuration from the current project. So, I am currently disabling skywalking's checkstyle execution by deactivating it.
@@ -38,8 +38,8 @@ We also provide [example compose files](examples/README.md) that integrate colle | |||
such as Kafka or Elasticsearch. | |||
|
|||
## Configuration | |||
Configuration is via environment variables, defined by [zipkin-server](https://github.com/openzipkin/zipkin/blob/master/zipkin-server/README.md). Notably, you'll want to look at the `STORAGE_TYPE` environment variables, which | |||
include "cassandra", "mysql" and "elasticsearch". | |||
Configuration is via environment variables, defined by [zipkin-server](https://github.com/openzipkin/zipkin/blob/master/zipkin-server/README.md). Notably, you'll want to look at the `ZIPKIN_STORAGE` environment variables, which |
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.
While I don't question the naming change I want to make sure we support BOTH to ease migration.
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.
@mrproliu We could change this back. I think.
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.
Got it, I will make these environments both work.
@@ -69,7 +69,7 @@ For example, to increase heap size, set `JAVA_OPTS` as shown in our [docker-comp | |||
|
|||
For example, to add debug logging, set `command` as shown in our [docker-compose](examples/docker-compose.yml) file: | |||
```yaml | |||
command: --logging.level.zipkin2=DEBUG | |||
command: -Dlog.level=DEBUG |
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 we support previous option too?
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 seems a little hard. log4j V.S. log4js. Unless we hard coded some hacking codes.
Changes look OK to me but I will wait for others to review. The only thing that catched my eye strongly was the env vars renaming. While this makes sense in some cases that I could identify, we must make sure migration isn't that much of a pain to users (that will have to drop their existing database anyways) and make sure old zipkin config remains working. One of the most important principles in entire zipkin ecosystem was to not to break sites even on major version changes, instead deprecate them in a major and remove them in the next major. |
So let's not rush to land this in main branch. I'd rather do release candidates with some real effort of reaching out to community and site owners (including a serious mention about this on our website). When we've had sufficient positive feedback cycles, concerns dealt with, enough broadcasting to reach our users we can talk about planning an official release and moving to main branch. |
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.
Let's give this more time and more communication around this effort. We need actual users to try and respond before an official release.
This also means providing notice and broadcasting about this effort on website, gitter, google groups, etc. As well as providing RC builds to be downloadable from site, docker hub, etc.
So, is it fine to release one from a branch? If this is OK, we can work on that way. |
IMHO it is fine to do release candidates / tech previews from a branch. Not sure how much work it will be to make it available. I just don't feel it's right to merge to main branch prior to the broader community approval. I want this effort to be correct and supported by the broader community. The impact is too large to swiftly go through without giving more time and consideration. |
OK, let me do this. (Release RCs from the branch) |
Once you can release an image we could test the automation in
https://github.com/openzipkin/zipkin-php-example as smoke test first.
José Carlos Chávez
ons. 15. nov. 2023 kl. 10:45 skrev 吴晟 Wu Sheng ***@***.***>:
… Closed #3594 <#3594>.
—
Reply to this email directly, view it on GitHub
<#3594 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAUZOWYXLVMZGJ6AR7DYESFLPAVCNFSM6AAAAAA7MEH5RSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQHE3DKMJYGQYTIMY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
We are planning to run a 3.0.0-rc0 release as we have finished all features covered in v3.\
The key features are as follows