- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9
Metrics with Prometheus #249
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
base: main
Are you sure you want to change the base?
Conversation
92e4094    to
    2965278      
    Compare
  
    c0da60a    to
    4a2b772      
    Compare
  
    b0a8d71    to
    1d8b660      
    Compare
  
    cad437c    to
    e28651a      
    Compare
  
    c6080e5    to
    9fcaa61      
    Compare
  
    9fcaa61    to
    48a6703      
    Compare
  
    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 is a review only on the packages config, config/generate and node/config.
Lets separate this PR in two steps: the first one prepares just the config aspects (what is reviewed here), and a second PR on top of that with all the metrics stuff.
| RequestMaxBytes: config.SharedConfig.BatchingConfig.RequestMaxBytes, | ||
| ClientSignatureVerificationRequired: config.LocalConfig.NodeLocalConfig.GeneralConfig.ClientSignatureVerificationRequired, | ||
| Bundle: bundle, | ||
| MonitoringListenAddress: config.LocalConfig.NodeLocalConfig.GeneralConfig.ListenAddress + ":" + strconv.Itoa(int(config.LocalConfig.NodeLocalConfig.GeneralConfig.MonitoringListenPort)), | 
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.
Lets have a separate listen address as well, define it in the local config.
Sometime we don't want the monitoring service to be exposed on the same address as the communication services, e.g. we want it on 127.0.01 while we set the communication interfaces to something public.
If the Monitoring address is missing (empty), use  the regular GeneralConfig.ListenAddress.
| SubmitTimeout: config.LocalConfig.NodeLocalConfig.BatcherParams.SubmitTimeout, | ||
| BatchSequenceGap: types.BatchSequence(config.LocalConfig.NodeLocalConfig.BatcherParams.BatchSequenceGap), | ||
| ClientSignatureVerificationRequired: config.LocalConfig.NodeLocalConfig.GeneralConfig.ClientSignatureVerificationRequired, | ||
| MonitoringListenAddress: config.LocalConfig.NodeLocalConfig.GeneralConfig.ListenAddress + ":" + strconv.Itoa(int(config.LocalConfig.NodeLocalConfig.GeneralConfig.MonitoringListenPort)), | 
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.
a separate listen address
| SigningPrivateKey: signingPrivateKey, | ||
| WALDir: DefaultConsenterNodeConfigParams(config.LocalConfig.NodeLocalConfig.FileStore.Path).WALDir, | ||
| BFTConfig: BFTConfig, | ||
| MonitoringListenAddress: config.LocalConfig.NodeLocalConfig.GeneralConfig.ListenAddress + ":" + strconv.Itoa(int(config.LocalConfig.NodeLocalConfig.GeneralConfig.MonitoringListenPort)), | 
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.
a separate listen address
| Consenter: consenterFromMyParty, | ||
| UseTLS: config.LocalConfig.TLSConfig.Enabled, | ||
| ClientAuthRequired: config.LocalConfig.TLSConfig.ClientAuthRequired, | ||
| MonitoringListenAddress: config.LocalConfig.NodeLocalConfig.GeneralConfig.ListenAddress + ":" + strconv.Itoa(int(config.LocalConfig.NodeLocalConfig.GeneralConfig.MonitoringListenPort)), | 
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.
a separate listen address
| // ListenPort is the port on which to bind to listen | ||
| ListenPort uint32 `yaml:"ListenPort,omitempty"` | ||
| // MonitoringListenPort is the port on which to expose the monitoring service | ||
| MonitoringListenPort uint32 `yaml:"MonitoringListenPort,omitempty"` | 
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.
Lets also have MonitoringListenAddress.
Lets have a separate listen address as well, defined here.
Sometime we don't want the monitoring service to be exposed on the same interface as the communication services, e.g. we want it on 127.0.01 while we set the communication interfaces to something public.
If the MonitoringListenAddress address is missing (empty), use the regular GeneralConfig.ListenAddress.
aef3dc3    to
    da9f901      
    Compare
  
    Signed-off-by: Genady Gurevich <genadyg@il.ibm.com>
da9f901    to
    00c09db      
    Compare
  
    
issue #185