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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Zipkin with Jaeger #94

Closed
wants to merge 21 commits into from
Closed

Replace Zipkin with Jaeger #94

wants to merge 21 commits into from

Conversation

k-rtik
Copy link
Contributor

@k-rtik k-rtik commented May 20, 2020

No description provided.

Copy link
Member

@gkwan-ibm gkwan-ibm left a comment

Choose a reason for hiding this comment

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

  • resolve the branch conflicts
  • What you’ll learn
    • Need to explain what is Jaeger in
  • because using docker, need an additional prereq section
  • Getting started
    • Better to have a subsection to start Jaeger after the following line:
      • "The finish directory contains the finished project that you will build."
    • move the following statemnts to "What you’ll learn"
      • "For this guide, use Jaeger as your distributed tracing system."
      • "You can find more information about Jaeger in their Getting Started docs."
    • dup "Try what you’ll build"
    • I don't think mvn liberty:start -pl system,inventory will work. Need to run mvn liberty:run -pl system and mvn liberty:start -pl inventory by 2 different shells
    • change liberty-maven-plugin to 3.2.1
    • need add more instructions how to get the spans. i.e. select inventory service, click the Find Traces button, etc
    • instuctions to stop inventory and system services
  • Existing Tracer implementation TODO
    • pom.xml better hotspot the specific lines for jagaer/opentracing, even need rewrite this paragraph, i.e. "This feature" means what?
      • server.xml better hotspot the specific lines for jagaer/opentracing
    • need to explain <classloader apiTypeVisibility="+third-party"/>?
    • the doc link "Enabling distributed tracing" is not good anymore. Ask Felix any better doc link. I doubt no need for this statemment because no need to install usr:opentracingZipkin-0.31
  • Enabling distributed tracing
    • Enabling distributed tracing without code instrumentation
      • "Both of these features are already enabled ..." not right now, remove and combine to the above paragraph
    • Enabling explicit distributed tracing
      • dev mode, no need to run mvn compile, there some places still have it
      • "...and sort the traces by newest first. " not clear enough what to do
      • get:io.openliberty.guides.inventory.inventoryresource.listcontents and inventorymanager.list not same as what I saw in the jagaer server. Make sure to update all outputs:
        • GET:io.openliberty.guides.inventory.InventoryResource.listContents
        • InventoryManager.list
      • "Again, run the mvn compile ..." no need to compile
  • Testing the services
    • need instruction to stop inventory, system services and jagaer server

@gkwan-ibm
Copy link
Member

gkwan-ibm commented May 28, 2020

@k-rtik

@gkwan-ibm
Copy link
Member

gkwan-ibm commented Jun 4, 2020

  • What you'll learn
    • "Opentracing" -> "OpenTracing" at the Jaeger paragraph
    • should have a link to Jaeger open source project?
    • move the Jaeger paragraph before "You will configure ..."
    • merge "For this guide, you will use Jaeger as your distributed tracing system." into somewhere in "You will configure ..." paragraph
      • "For this guide," is no need
  • Starting Jaeger
    • docs -> documentation
    • this subsection should be outside of (i.e. not under) "Try what you’ll build" subsection.
  • Try what you’ll build
    • "To try out the application, ..." this paragraph is not clear to me. Suggest something like:
      • Open a command line session and navigate to the finish/inventory directory
      • mvn liberty:run
      • Open another command line session and navigate to the finish/system directory
      • mvn liberty:run
    • "If you only see the jaeger-service listed..." I saw jaeger-query instead of jaeger-service
    • "The trace should have 4 spans,..." I saw 3 spans instead of 4
    • Consider to remove "Verify that there are three spans from inventory: - ..." and "Verify that there is one span from system: - "
      • otherwise, descibe them clearer
    • "After you are finished checking out the application, stop the Open Liberty server..." -> "After you are finished checking out the application, stop both Open Liberty servers"
      • probably have to clone from the common adoc instead of include it
  • Existing Tracer implementation
    • the hotspot for pom.xml should highlight jaeger-client dependency
  • Enabling distributed tracing
    • In paragraph "To add the availability ...package types supported.",
      • hotspot for <classLoader /> not work
      • add hotspot for apiTypeVisibility although highlight the same line as <classLoader />
    • why InventoryResource.java has the line // @Traced(false)?
    • "You see a new trace record that is two spans long with one span for the listContents()" I saw 3 spans instead of 2
    • after replace InventoryManager.java, I could not see "The system trace:" described
  • Testing the services
    • should not start dev mode
    • rewrite "When you are done checking out the service, exit development mode by pressing ..." to make it clear to stop both inventory and system services
  • try the guide with all 3 platforms
    • try the guide with a new clone instead of using your development environment

@k-rtik k-rtik requested a review from gkwan-ibm June 4, 2020 20:25
@gkwan-ibm
Copy link
Member

gkwan-ibm commented Jun 4, 2020

  • Additional prerequisites
    • do not make "Deploying Jaeger" as as subsection. Just bold it.
  • Try what you’ll build
    • "Run the following Maven goal to build the service and deploy it to Open Liberty:" -> "Run the following Maven goal to build the inventory service and deploy it to Open Liberty:"
    • "Run the following Maven goal to build the services and deploy them to Open Liberty:" -> "Run the following Maven goal to build the system service and deploy it to Open Liberty:"
    • "After you see the following message, your application server is ready." -> "After you see the following message, both services are ready.
    • I guess mvn liberty:stop will work if run at the finish directory instead of mvn -pl system,inventory liberty:stop
  • Testing the services
    • dup "Running the tests"
    • "When you are done checking out the service, exit development mode by pressing CTRL+C, or by typing q and then pressing the enter/return key. in the shell sessions where you ran the system and inventory services." -> "When you are done checking out the services, exit development mode by pressing CTRL+C in the shell sessions where you ran the system and inventory services, , or by typing q and then pressing the enter/return key ."

@MaiHameed MaiHameed mentioned this pull request Jun 10, 2020
47 tasks
k-rtik and others added 3 commits June 11, 2020 14:03
* linux peer review

* mac peer review

* slf4j dependencies

* OS tabs for docker line breaks
…acing into jaeger

# Conflicts:
#	README.adoc
@gkwan-ibm gkwan-ibm added the invalid This doesn't seem right label Sep 11, 2020
@gkwan-ibm
Copy link
Member

This PR was addressed by the new guide https://openliberty.io/guides/microprofile-opentracing-jaeger.html

@gkwan-ibm gkwan-ibm closed this May 26, 2021
@gkwan-ibm gkwan-ibm deleted the branch qa May 26, 2021 19:54
@gkwan-ibm gkwan-ibm deleted the jaeger branch September 16, 2022 13:53
@gkwan-ibm gkwan-ibm restored the jaeger branch September 16, 2022 13:53
@gkwan-ibm gkwan-ibm deleted the jaeger branch August 30, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants