Skip to content

Conversation

@fmorency
Copy link
Contributor

@fmorency fmorency commented Jun 5, 2025

This pull request introduces enhancements to the GeoIP functionality of the Prometheus exporter, including the integration of IPBase for GeoIP lookups, state persistence for caching, and improved configuration options. The most important changes are grouped below by theme.

GeoIP Enhancements:

  • Updated GeoIPCollector to support IPBase API integration by adding an ipbase-key parameter and refactoring the getGeoIP function to use the IPBase API. [1] [2]
  • Added caching functionality to GeoIPCollector, enabling state persistence for GeoIP data with a configurable state-file parameter. This includes methods to load and save state, and logic to refresh data based on cache expiration. [1] [2]

Configuration Updates:

  • Added new flags --ipbase-key and --state-file to the serve command for configuring the IPBase API key and state file path, respectively.
  • Updated ServeConfig to include IpBaseKey and StateFile fields, with validation to ensure state-file is specified. [1] [2]

Code Refactoring:

  • Refactored the GeoIP data structures to align with the IPBase API response, introducing new types such as GeoIPResponse, GeoIPData, and GeoIPLocation.
  • Replaced hardcoded FreeGeoIP URL with IPBase API endpoint in getGeoIP.

Documentation:

  • Updated README.md to document the new --ipbase-key and --state-file flags.

@fmorency fmorency requested a review from Copilot June 5, 2025 14:31
@fmorency fmorency self-assigned this Jun 5, 2025
@fmorency fmorency requested a review from jgryffindor June 5, 2025 14:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ports GeoIP functionality to use the IPBase API, introduces caching for GeoIP data with state persistence, and updates configuration and documentation accordingly.

  • GeoIPCollector now integrates with the IPBase API and supports caching.
  • New configuration flags (––ipbase-key, ––state-file) were added to enable these features.
  • Documentation in README.md has been updated to reflect the new settings.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
pkg/config.go Added new configuration fields and validation for IPBase key and state file.
pkg/collectors/geoip.go Refactored GeoIPCollector to integrate IPBase API, caching logic added.
cmd/manifest-node-exporter/serve.go Updated collector instantiation and added new CLI flags for configuration.
README.md Updated documentation with new CLI flag details.
Comments suppressed due to low confidence (3)

pkg/config.go:30

  • Currently, the configuration validation enforces a non-empty state-file even when the GeoIP integration may be disabled by the absence of an IPBase key. Consider making the state-file requirement conditional on IPBase integration being enabled.
if c.StateFile == "" {

pkg/config.go:13

  • [nitpick] For consistency with common Go naming conventions for acronyms, consider renaming 'IpBaseKey' to 'IPBaseKey'.
IpBaseKey     string `mapstructure:"ipbase_key"`

pkg/collectors/geoip.go:173

  • [nitpick] It would be helpful to add a comment explaining the rationale for using a random jitter over the one-month duration to clarify its intent and expected range.
jitter := time.Duration(rand.Int63n(int64(dur)))

@jgryffindor
Copy link

Tested ACK, looks good!

@fmorency fmorency merged commit e379e63 into manifest-network:main Jun 6, 2025
3 checks passed
@fmorency fmorency deleted the ipbase branch June 6, 2025 12:13
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.

2 participants