Skip to content

Conversation

@JStickler
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@objectiser
Copy link

@pavolloffay @kevinearls @rubenvp8510 @jkandasa
If you could all take a look and provide Julie some feedback - still work in progress but would be good to give some overall comments on the structure and general content. Thanks.

@JStickler JStickler force-pushed the BOJ_03 branch 4 times, most recently from 60590b9 to 7c722a7 Compare March 26, 2020 17:26
Copy link

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Will comment more later/tomorrow - have some meetings.

@JStickler JStickler force-pushed the BOJ_03 branch 2 times, most recently from 2fc795a to 87af14a Compare March 27, 2020 01:29

Choose a reason for hiding this comment

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

As we only support self provisioned ES clusters, the location of the ES cluster is not defined. However for the next version it will be supported (i.e. using external cluster) so need to take into account this will change for next release.

This is also an area where it is dependent upon the deployment strategy - i.e. if production then would be dealing with ES config, but if streaming then dealing with Kafka config.

Choose a reason for hiding this comment

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

I see that the ES config options are described in a separate section. Possibly the Kafka options should also be described in a separate section (both showing the collector's kafka config and the ingester's kafka config) - with links from this section based on the strategy?

Then the options in this section only need to focus on the collector specific options, without worrying about the storage/kafka - which are strategy dependent.

In this case, there are additional collector options that need to be discussed: see https://www.jaegertracing.io/docs/1.17/cli/#jaeger-collector - this provides the options based on the storage chosen, but you should be able to pick any of them and just look for the options that are no related to that storage. Also don't include options that are marked as deprecated or experimental(?).

Choose a reason for hiding this comment

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

Just to be clear - the CLI flags section identifies the options which are defined in yaml in the options sections - but not all will be relevant. For example https://www.jaegertracing.io/docs/1.17/cli/#jaeger-collector-elasticsearch shows the CLI flags that are relevant when using the collector with elasticsearch storage. However the elasticsearch flags (es or es-archive prefixed) will need to be documented in the storage section, but others can be defined (with some exceptions) in the collector options node.

The general convention is usually to define the tree structure based on the '.', so --collector.grpc.tls.cert would be:

collector:
  options:
    collector:
      grpc:
        tls:
          cert: value

The exceptions that don't need to be documented are things like --help, --config-file, --sampling.strategies-file (as this is handled in the sampling config) and --span-storage.type (as handled in the storage section).

Copy link

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Thanks Julie - great start.

Seems to be missing sections for configuring UI and agent (e.g. sidecar injection, etc).

Copy link

@objectiser objectiser Mar 27, 2020

Choose a reason for hiding this comment

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

These options are not being laid out the same as with the collector and storage sections.

Same as with collector and storage, there are other relevant options defined here: https://www.jaegertracing.io/docs/1.17/cli/#jaeger-query

Choose a reason for hiding this comment

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

@pavolloffay Isn't the ES self provisioned instance created in the same namespace as the Jaeger instance?

Choose a reason for hiding this comment

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

Don't think we should have this comment - @pavolloffay can you confirm?

@objectiser
Copy link

@JStickler One additional thing we will need to sort out - currently Jaeger operator only has a single "stable" channel so users don't have a choice. As of "RHOSJ 1.17" there will be maintenance (z-stream) channels for each version, to support Service Mesh require for users being able to stay on a particular version but get bug fixes and CVEs. So we may have "v1.13" and "v1.17" (not 100% sure of the channel names yet) - but will need to explain what the different channels represent.

@objectiser
Copy link

Hi @JStickler would you be able to push the next set of updates so we can continue reviewing? Thanks.

@JStickler JStickler changed the title [WIP] Jaeger configuration and deployment options Jaeger configuration and deployment options Apr 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020
@objectiser
Copy link

Hi @JStickler, there are still many previous comments that haven't been addressed. Are you going to be addressing them before the first version of the docs will be released? If so, let me know once they are updated and I'll review at that point.

In the meantime I will review the new sidecar sections.

Copy link

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Couldn't find the location in the table of contents for these pages?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020
@JStickler
Copy link
Contributor Author

@objectiser , @pavolloffay , @kevinearls @jkandasa When you have time, if I could get another round of reviews (and hopefully approvals) on this PR.

I'll be starting a new PR to address the comments about documenting additional options, because I don't want to hold up this PR.

@JStickler JStickler force-pushed the BOJ_03 branch 9 times, most recently from dd39ce0 to 29d72be Compare May 19, 2020 13:48
@JStickler JStickler force-pushed the BOJ_03 branch 2 times, most recently from 6dd335f to 0bba47d Compare May 19, 2020 19:40
@JStickler JStickler merged commit 75aa180 into openshift:master May 19, 2020
neal-timpe added a commit that referenced this pull request Jun 3, 2020
Jaeger configuration and deployment options (#20643 restored)
neal-timpe added a commit that referenced this pull request Jun 3, 2020
…-22296-to-enterprise-4.4

[enterprise-4.4] Jaeger configuration and deployment options (#20643 restored)
neal-timpe added a commit that referenced this pull request Jun 3, 2020
…-22296-to-enterprise-4.5

[enterprise-4.5] Jaeger configuration and deployment options (#20643 restored)
@JStickler JStickler deleted the BOJ_03 branch November 11, 2020 15:54
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.

8 participants