From e2cf7e1cdaec6efd19c841e79593e73d0e111735 Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Fri, 13 Oct 2023 12:03:54 -0500 Subject: [PATCH 1/2] Reduce log spam: gated logging for high frequency items. --- .../price/spot/ArsBlueRateTransformer.java | 6 +-- .../bisq/price/spot/ExchangeRateProvider.java | 5 +- .../bisq/price/spot/ExchangeRateService.java | 35 ++++++++------ .../java/bisq/price/util/GatedLogging.java | 46 +++++++++++++++++++ 4 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 src/main/java/bisq/price/util/GatedLogging.java diff --git a/src/main/java/bisq/price/spot/ArsBlueRateTransformer.java b/src/main/java/bisq/price/spot/ArsBlueRateTransformer.java index 5d6b2f9..f3ac069 100644 --- a/src/main/java/bisq/price/spot/ArsBlueRateTransformer.java +++ b/src/main/java/bisq/price/spot/ArsBlueRateTransformer.java @@ -19,14 +19,12 @@ import bisq.price.spot.providers.BlueRateProvider; import bisq.price.util.bluelytics.ArsBlueMarketGapProvider; -import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Component; import java.util.Optional; import java.util.OptionalDouble; @Component -@Slf4j public class ArsBlueRateTransformer implements ExchangeRateTransformer { private final ArsBlueMarketGapProvider blueMarketGapProvider; @@ -54,8 +52,8 @@ public Optional apply(ExchangeRateProvider provider, ExchangeRate originalExchangeRate.getProvider() ); - log.info(String.format("%s transformed from %s to %s", - originalExchangeRate.getCurrency(), originalExchangeRate.getPrice(), blueRate)); + provider.getGatedLogging().maybeLogInfo(String.format("%s transformed from %s to %s", + originalExchangeRate.getCurrency(), originalExchangeRate.getPrice(), blueRate)); return Optional.of(newExchangeRate); } diff --git a/src/main/java/bisq/price/spot/ExchangeRateProvider.java b/src/main/java/bisq/price/spot/ExchangeRateProvider.java index d442a51..5dafb61 100644 --- a/src/main/java/bisq/price/spot/ExchangeRateProvider.java +++ b/src/main/java/bisq/price/spot/ExchangeRateProvider.java @@ -20,6 +20,8 @@ import bisq.core.locale.CurrencyUtil; import bisq.core.locale.TradeCurrency; import bisq.price.PriceProvider; +import bisq.price.util.GatedLogging; +import lombok.Getter; import org.knowm.xchange.Exchange; import org.knowm.xchange.ExchangeFactory; import org.knowm.xchange.currency.Currency; @@ -32,7 +34,6 @@ import org.knowm.xchange.service.marketdata.params.Params; import org.springframework.core.env.Environment; -import javax.annotation.Nullable; import java.io.IOException; import java.math.BigDecimal; import java.math.RoundingMode; @@ -59,6 +60,8 @@ public abstract class ExchangeRateProvider extends PriceProvider providers; private final List transformers; + private final GatedLogging gatedLogging = new GatedLogging(); /** * Construct an {@link ExchangeRateService} with a list of all @@ -87,6 +88,7 @@ public Map getAllMarketPrices() { * by currency code */ private Map getAggregateExchangeRates() { + boolean maybeLogDetails = gatedLogging.gatingOperation(); Map aggregateExchangeRates = new HashMap<>(); // Query all providers and collect all exchange rates, grouped by currency code @@ -110,7 +112,7 @@ private Map getAggregateExchangeRates() { } else { // If multiple providers have rates for this currency, then // aggregate = average of the rates - double priceAvg = priceAverageWithOutliersRemoved(exchangeRateList, currencyCode); + double priceAvg = priceAverageWithOutliersRemoved(exchangeRateList, currencyCode, maybeLogDetails); aggregateExchangeRate = new ExchangeRate( currencyCode, BigDecimal.valueOf(priceAvg), @@ -123,7 +125,8 @@ private Map getAggregateExchangeRates() { return aggregateExchangeRates; } - private double priceAverageWithOutliersRemoved(List exchangeRateList, String contextInfo) { + private double priceAverageWithOutliersRemoved( + List exchangeRateList, String contextInfo, boolean logOutliers) { final List yValues = exchangeRateList.stream(). mapToDouble(ExchangeRate::getPrice).boxed().collect(Collectors.toList()); Tuple2 tuple = InlierUtil.findInlierRange(yValues, 0, getOutlierStdDeviation()); @@ -145,16 +148,18 @@ private double priceAverageWithOutliersRemoved(List exchangeRateLi double priceAvg = opt.orElseThrow(IllegalStateException::new); // log the outlier prices which were removed from the average, if any. - for (ExchangeRate badRate : exchangeRateList.stream() - .filter(e -> !filteredPrices.contains(e)) - .collect(Collectors.toList())) { - log.info("{} {} outlier price removed:{}, lower/upper bounds:{}/{}, consensus price:{}", - badRate.getProvider(), - badRate.getCurrency(), - badRate.getPrice(), - lowerBound, - upperBound, - priceAvg); + if (logOutliers) { + for (ExchangeRate badRate : exchangeRateList.stream() + .filter(e -> !filteredPrices.contains(e)) + .collect(Collectors.toList())) { + log.info("{} {} outlier price removed:{}, lower/upper bounds:{}/{}, consensus price:{}", + badRate.getProvider(), + badRate.getCurrency(), + badRate.getPrice(), + lowerBound, + upperBound, + priceAvg); + } } return priceAvg; } diff --git a/src/main/java/bisq/price/util/GatedLogging.java b/src/main/java/bisq/price/util/GatedLogging.java new file mode 100644 index 0000000..b4070cd --- /dev/null +++ b/src/main/java/bisq/price/util/GatedLogging.java @@ -0,0 +1,46 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.price.util; + +import lombok.extern.slf4j.Slf4j; + +import java.time.Instant; + +/* Per https://github.com/bisq-network/bisq-pricenode/issues/33 + * There's too much logging of outlier filtering data, fills up the logs too fast and obliterates other valid logging. + * It correlates with client requests. Change that logging so its output once per minute. + */ + +@Slf4j +public class GatedLogging { + private long timestampOfLastLogMessage = 0; + + public void maybeLogInfo(String format, Object... params) { + if (gatingOperation()) { + log.info(format, params); + } + } + + public boolean gatingOperation() { + if (Instant.now().getEpochSecond() - timestampOfLastLogMessage > 60) { + timestampOfLastLogMessage = Instant.now().getEpochSecond(); + return true; + } + return false; + } +} From 2d00f255942bb81d89e69841551226815dcc5e95 Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Sat, 14 Oct 2023 22:32:04 -0500 Subject: [PATCH 2/2] Remove stale prices that have not been updated for over 10 minutes. --- src/main/java/bisq/price/PriceProvider.java | 6 +++- .../bisq/price/spot/ExchangeRateProvider.java | 18 ++++++++++ .../bisq/price/spot/ExchangeRateService.java | 1 + .../price/spot/ExchangeRateServiceTest.java | 34 +++++++++++++++++-- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/main/java/bisq/price/PriceProvider.java b/src/main/java/bisq/price/PriceProvider.java index cbeefd9..f4e4981 100644 --- a/src/main/java/bisq/price/PriceProvider.java +++ b/src/main/java/bisq/price/PriceProvider.java @@ -51,6 +51,10 @@ public final T get() { return cachedResult; } + public final void put(T values) { + cachedResult = values; + } + @Override public final void start() { // do the initial refresh asynchronously @@ -80,7 +84,7 @@ public void run() { private void refresh() { long ts = System.currentTimeMillis(); - cachedResult = doGet(); + put(doGet()); log.info("refresh took {} ms.", (System.currentTimeMillis() - ts)); diff --git a/src/main/java/bisq/price/spot/ExchangeRateProvider.java b/src/main/java/bisq/price/spot/ExchangeRateProvider.java index 5dafb61..56fe747 100644 --- a/src/main/java/bisq/price/spot/ExchangeRateProvider.java +++ b/src/main/java/bisq/price/spot/ExchangeRateProvider.java @@ -39,6 +39,7 @@ import java.math.RoundingMode; import java.time.Duration; import java.util.*; +import java.util.concurrent.TimeUnit; import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -54,6 +55,7 @@ */ public abstract class ExchangeRateProvider extends PriceProvider> { + private static final long STALE_PRICE_INTERVAL_MILLIS = TimeUnit.MINUTES.toMillis(10); private static Set SUPPORTED_CRYPTO_CURRENCIES = new HashSet<>(); private static Set SUPPORTED_FIAT_CURRENCIES = new HashSet<>(); private final Set providerExclusionList = new HashSet<>(); @@ -131,6 +133,22 @@ public String getPrefix() { return prefix; } + public void maybeClearStaleRates() { + // a stale rate is older than the specified interval, except: + // timestamp of 0L is used as special case re: CoinMarketCap and BitcoinAverage + // (https://github.com/bisq-network/bisq-pricenode/issues/23) + long staleTimestamp = new Date().getTime() - STALE_PRICE_INTERVAL_MILLIS; + Set nonStaleRates = get().stream() + .filter(e -> e.getTimestamp() == 0L || e.getTimestamp() > staleTimestamp) + .collect(Collectors.toSet()); + long numberOriginalRates = get().size(); + if (numberOriginalRates > nonStaleRates.size()) { + put(nonStaleRates); + log.warn("{} {} stale rates removed, now {} rates", + getName(), numberOriginalRates, nonStaleRates.size()); + } + } + @Override protected void onRefresh() { get().stream() diff --git a/src/main/java/bisq/price/spot/ExchangeRateService.java b/src/main/java/bisq/price/spot/ExchangeRateService.java index 7b48f21..ff8322b 100644 --- a/src/main/java/bisq/price/spot/ExchangeRateService.java +++ b/src/main/java/bisq/price/spot/ExchangeRateService.java @@ -60,6 +60,7 @@ public Map getAllMarketPrices() { Map aggregateExchangeRates = getAggregateExchangeRates(); providers.forEach(p -> { + p.maybeClearStaleRates(); // Specific metadata fields for specific providers are expected by the client, // mostly for historical reasons // Therefore, add metadata fields for all known providers diff --git a/src/test/java/bisq/price/spot/ExchangeRateServiceTest.java b/src/test/java/bisq/price/spot/ExchangeRateServiceTest.java index a5bc01b..45796bd 100644 --- a/src/test/java/bisq/price/spot/ExchangeRateServiceTest.java +++ b/src/test/java/bisq/price/spot/ExchangeRateServiceTest.java @@ -165,6 +165,28 @@ public void getAllMarketPrices_withMultipleProviders_overlappingCurrencyCodes() assertNotEquals(0L, retrievedData.get(providers.get(1).getPrefix() + "Ts")); } + @Test + public void testStaleRatesRemoved() { + String fiatCoin = "BRL"; + // insert a stale rate + long staleTimestamp = 10000L; + Long validTimestamp = System.currentTimeMillis(); + List providers = asList( + buildDummyExchangeRateProviderWithRateAndTimestamp("mercadoBitcoin", fiatCoin, 129000.0, staleTimestamp), + buildDummyExchangeRateProviderWithRateAndTimestamp("coinGecko", fiatCoin, 129000.0, validTimestamp), + buildDummyExchangeRateProviderWithRateAndTimestamp("binance", fiatCoin, 131000.0, validTimestamp)); + Map retrievedData = new ExchangeRateService(new StandardEnvironment(), providers, Collections.emptyList()).getAllMarketPrices(); + doSanityChecksForRetrievedDataMultipleProviders(retrievedData, providers); + // check that the provider's stale rate was removed resulting in 0 rates and a 0 timestamp + assertEquals("0", retrievedData.get("mercadoBitcoinTs").toString()); + assertEquals("0", retrievedData.get("mercadoBitcoinCount").toString()); + // check that other providers are unaffected + assertEquals("1", retrievedData.get("coinGeckoCount").toString()); + assertEquals("1", retrievedData.get("binanceCount").toString()); + assertEquals(validTimestamp.toString(), retrievedData.get("coinGeckoTs").toString()); + assertEquals(validTimestamp.toString(), retrievedData.get("binanceTs").toString()); + } + @Test public void bisqIndexCalculation_oneOutlierPriceWideRange() { String fiatCoin = "BRL"; @@ -517,7 +539,13 @@ protected Set doGet() { return dummyProvider; } - private ExchangeRateProvider buildDummyExchangeRateProviderWithRate(String providerName, String currencyCode, Double dummyRate) { + private ExchangeRateProvider buildDummyExchangeRateProviderWithRate( + String providerName, String currencyCode, Double rate) { + return buildDummyExchangeRateProviderWithRateAndTimestamp( + providerName, currencyCode, rate, System.currentTimeMillis()); + } + private ExchangeRateProvider buildDummyExchangeRateProviderWithRateAndTimestamp( + String providerName, String currencyCode, Double rate, long timestamp) { ExchangeRateProvider dummyProvider = new ExchangeRateProvider( new StandardEnvironment(), providerName, @@ -534,8 +562,8 @@ protected Set doGet() { HashSet exchangeRates = new HashSet<>(); exchangeRates.add(new ExchangeRate( currencyCode, - dummyRate, - System.currentTimeMillis(), + rate, + timestamp, getName())); // ExchangeRateProvider name return exchangeRates; }