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

Closes #5 - Add Kubernetes demo with inspectIT Ocelot and OpenTelmetry Collector #7

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

heiko-holz
Copy link
Contributor

@heiko-holz heiko-holz commented Apr 12, 2022

Closes #5


This change is Reviewable

@heiko-holz heiko-holz changed the title Closes #4 - Add Kubernetes demo with inspectIT Ocelot and OpenTelmetry Collector Closes #5 - Add Kubernetes demo with inspectIT Ocelot and OpenTelmetry Collector Apr 12, 2022
Copy link
Member

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

Reviewed 67 of 67 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @heiko-holz)


trading-application-k8s-demo/README.md, line 24 at r1 (raw file):

      ```

   2. Install [Ingress](https://kubernetes.github.io/ingress-nginx/deploy/)

Since Ingress is not mentioned in the operator readme, should it maybe moved somewhere else? What is it needed for anyways? For me it worked without it too.


trading-application-k8s-demo/README.md, line 53 at r1 (raw file):

### 1. Using the Jaeger Operator

1. Install the [Jaeger Operator](https://www.jaegertracing.io/docs/1.32/operator/) using the following commands. Please note that his will install the latest version of it:

The operator expects the namespace to be 'observability' instead of 'monitoring' and because of this can not be installed fully


trading-application-k8s-demo/README.md, line 64 at r1 (raw file):

2. Deploy`Jaeger-all-in-one` in the `monitoring` namespace

   1. Create `jaeger-all-in-one.yaml` file

This file already exists in the jaeger folder, doesn't it?


trading-application-k8s-demo/README.md, line 85 at r1 (raw file):

   kind: Jaeger
   metadata:
     name: jaeger

Definitely could be me using it wrong, but I can not make this command work in Powershell at least. Since you have the jaeger-all-in-one.yaml in the repo, I personally don't think it is really needed anyways.


trading-application-k8s-demo/README.md, line 120 at r1 (raw file):

     ```

3. Use `port-forward` to access the Jaeger UI on localhost on port 16686 ([http://localhost:16686](http://localhost:16686))

This should probably be another ### heading instead, shouldn't it? Since it is needed for both standalone and operator version. Maybe same for the "2. Verify deployment" one.


trading-application-k8s-demo/README.md, line 424 at r1 (raw file):

### 2. Deploy the inspectIT Ocelot Configuration Server

We will deploy the inspectIT Ocelot Configuration Server following the [official readme](https://github.com/inspectIT/inspectit-ocelot/tree/master/components/inspectit-ocelot-configurationserver/k8s).

Did you plan to add a readme to that folder in the main repo? Because right now the link name is a bit confusing and then the link in general not really needed, since you have your own deployment yaml anyways.
You could also consider instead writing something like 'You can find the file in [...]. It is based on the official deployment example.'.


trading-application-k8s-demo/README.md, line 433 at r1 (raw file):

    ```
    
      * we configure inspectIT Ocelot to trace all methods from the trading demo frontend and backend

I would maybe write something like 'The deployment has a configuration added already that configures the inspectIT Ocelot agents to trace ...', because right now it was a bit confusing to me where this configuration comes from.


trading-application-k8s-demo/README.md, line 483 at r1 (raw file):

### 1. Deploy with OpenTelemetryCollector Agent as sidecar

The file `./trading-demo/trading-demo-with-otelcol-agent-sidecar.yaml` deploys the trading demo application with the annotation `sidecar.opentelemetry.io/inject: "true"` that tells the OpenTelemetry Operator to inject the OpenTelemetry Collector Agent as a sidecar

I'd consider adding the picture from the top here too, so it is easier to compare between the two possibilities.


trading-application-k8s-demo/README.md, line 575 at r1 (raw file):

<p align="center">
<img src="./images/2022-03-inspectit-ocelot-k8s-demo.svg" width="50%" alt="inspectIT Ocelot k8s demo"/>

I'd maybe add one sentence to explain what the picture shows, how it differs from the other version.


trading-application-k8s-demo/README.md, line 613 at r1 (raw file):

* Jaeger UI: [http://localhost:16686](http://localhost:16686)
* inspectIT Ocelot Configuration Server: [http://localhost:8090](http://localhost:8090)

maybe mention the username and password, because not everyone might know where to look


trading-application-k8s-demo/images/2022-03-inspectit-ocelot-k8s-demo.svg, line 0 at r2 (raw file):
I wonder if we need all the different versions of the image. The drawio one makes sense since one could modify it, but do we need the svgs for each too then?


trading-application-k8s-demo/images/2022-03-inspectit-ocelot-k8s-demo-sidecar.svg, line 0 at r2 (raw file):
See other svg question.


trading-application-k8s-demo/inspectit-ocelot/inspectit-ocelot-configurationserver.yaml, line 73 at r2 (raw file):

    spec:
      containers:
        - image: inspectit/inspectit-ocelot-configurationserver

I would probably add a tag here too, especially if the agent is not configured to latest, since it might happen in the future that the two would become incompatible.


trading-application-k8s-demo/inspectit-ocelot/inspectit-ocelot-instrumentation.yaml, line 7 at r2 (raw file):

spec:
  java:
    image: inspectit/inspectit-ocelot-agent:1.15.2

If there are no conflicts, I would update to 1.16. I assume latest would be too dangerous to break something and 1.16 is enough for the demo for the foreseeable future anyways?


trading-application-k8s-demo/otel/otel-collector.yaml, line 79 at r2 (raw file):

      jaeger: 
        # export to Jaeger in the namespace monitoring via gRPC
        endpoint: "jaeger.monitoring:14250"

Namespace would of course need to be changed when using the jaeger operator as well.
But also, the service name using the operator is different, it is jaeger-collector then.


trading-application-k8s-demo/otel/otel-collector-contrib.yaml, line 1 at r2 (raw file):

kind: Namespace

what is this file needed for? Since it is the same as the other otel-collector one except for the otel image and is not mentioned in the tutorial


trading-application-k8s-demo/otel/otel-instrumentation.yaml, line 1 at r2 (raw file):

apiVersion: opentelemetry.io/v1alpha1

what is this file needed for, since it is never mentioned in the readme or in any other file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] - Add Kubernetes demo with inspectIT Ocelot and OpenTelmetry Collector
2 participants