This repository was archived by the owner on Jun 24, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 43
Add metrics port to operator #216
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ac3c5fa
Added metrics port to operator yaml
jihuin 1b8e709
Merge remote-tracking branch 'upstream/master'
jihuin 86d46bd
Added stats reporter to emit metrics when reconciling knative serving
jihuin a75e3d4
Revert "Added stats reporter to emit metrics when reconciling knative…
jihuin 885f020
Sync with upstream
jihuin b8e0c3a
Changed the component name from 'serving_operator' to 'operator'
jihuin 568f225
Changed the component name to `serving_operator`
jihuin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 do not think we need to add CONFIG_LOGGING_NAME and CONFIG_OBSERVABILITY_NAME, anymore.
By default, knative/pkg will pick config-logging and config-observability.
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.
It's true that knative/pkg will watch config-logging and config-observability configmaps by default if nothing is specified. Maybe it is meaningful to add here to let the users be aware of the purposes of these two configmaps.
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.
Though the configmap filenames are quite self-explaining, the users can not see if or where the configmaps are watched unless digging into knative/pkg
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.
knative/pkg@9491151
I just had this PR merged. If config-logging and config-observability are missing, the controller should be launched without problem. With this PR, would it be fine, if config-logging and config-observability are missing??
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.
@jihuin again, you are correct here.
See knative/eventing@36058f4 for adding port (the
CONFIG_XYZ_NAMEwere already in the yamls, eg:https://github.com/knative/eventing/blob/master/config/channels/in-memory-channel/500-controller.yaml#L37-L40
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.
If the configmaps are missing and not watched, the exporter won't be set up and no metric is exported, but no information is provided in the logs. The configs here can be informative.