Skip to content
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

Ticker no Timestamp #544

Closed
eRfO opened this issue Apr 14, 2021 · 48 comments
Closed

Ticker no Timestamp #544

eRfO opened this issue Apr 14, 2021 · 48 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@eRfO
Copy link

eRfO commented Apr 14, 2021

Hi,
when the Ticker returrned by exchange has timestamp field to null, only the first value is posted to the strategy. This happens because the timestamp on the object is null and in the TickerFlux class there is a check on TickerDTO that compare the previuous timestamp with the new one but the getTimestamp method returns now for both instances.

Maybe you could set the timestamp directly in the mapper when the exchange doesn't.

Thanks,
Michele

@straumat
Copy link
Member

Hi Michele, which release of Cassandre do you use ?
I thought this was fixed in this issue : #482

@straumat straumat self-assigned this Apr 14, 2021
@straumat straumat added the bug Something isn't working label Apr 14, 2021
@eRfO
Copy link
Author

eRfO commented Apr 14, 2021

Hi Stéphane,
I'm using 4.1.1 version of Cassandre, it seems to be the last.

Thank you.

@straumat
Copy link
Member

Which exchange are you using ? do you have any errors in the logs ?

@eRfO
Copy link
Author

eRfO commented Apr 15, 2021

I'm using Binance, but seems that the errors is related to TickerDTO equals method. Decompiling code I see this:

public final boolean equals(final Object o) { if (this == o) { return true; } else if (o != null && this.getClass() == o.getClass()) { TickerDTO that = (TickerDTO)o; return (new EqualsBuilder()).append(this.currencyPair, that.currencyPair).append(this.getTimestamp(), that.getTimestamp()).isEquals(); } else { return false; } }

so the old code.

Thanks,
Michele

@straumat
Copy link
Member

@eRfO can you should me the first ticker received in your strategy?

@eRfO
Copy link
Author

eRfO commented Apr 15, 2021

Yes, I tried to run now and in the console I see:

New ticker TickerDTO(currencyPair=BTC/EUR, open=52590.25000000, last=53162.91000000, bid=53159.35000000, ask=53162.94000000, high=53404.90000000, low=51961.63000000, vwap=52600.01430987, volume=2713.52616600, quoteVolume=142731515.16180777, bidSize=0.00789300, askSize=0.00156300, timestamp=2021-04-16T00:13:05.207450700+02:00[Europe/Rome])

after this ticker nothing else.

@straumat
Copy link
Member

@eRfO Ok, I will create a binance account and make a test.

@straumat straumat added this to the 4.2.0 milestone Apr 15, 2021
@eRfO
Copy link
Author

eRfO commented Apr 15, 2021

Ok, thank you.

@Tempys
Copy link

Tempys commented Apr 16, 2021

@eRfO have the same issue (

@straumat
Copy link
Member

Hi, @eRfO & @Tempys I made a test and it's working. Maybe you should change the release number of XChange in your pom.xml?

This is what I set:

        <dependency>
            <groupId>org.knowm.xchange</groupId>
            <artifactId>xchange-binance</artifactId>
            <version>5.0.7</version>
        </dependency>

It works with my Binance integration test.

@Tempys
Copy link

Tempys commented Apr 16, 2021

@straumat it also works for me in dry mode but does not work when dry mode is false

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

Hi,
@straumat the tests I did already used the latest version of the XChange library, but in the compiled code I keep seeing the equals method of TickerDTO with only currencyPair and timestamp fields. Even as @Tempys I don't use dry mode.

@Tempys
Copy link

Tempys commented Apr 16, 2021

@straumat @eRfO basically after updating version to latest version i start get to event but no so often as should be .

@straumat
Copy link
Member

@Tempys it's "normal". In fact, if you ask Cassandre to check tickers every second for example, it does it. But if the server replis with the same value, cassandre will only push data if they are different from the previous one received

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

image

@straumat as you can see, just only one ticker is emitted by the TickerFlux, I'm using BTC/EUR currency pair in my test.
The ticker rate configuration is set to:

cassandre.trading.bot.exchange.rates.ticker=PT5S

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

image

Just a question, the above change is in 4.1.1 version?

Because in the code of version 4.1.1 I keep seeing the equals method written as follows:

 public final boolean equals(final Object o) {
    if (this == o) {
        return true;
    } else if (o != null && this.getClass() == o.getClass()) {
        TickerDTO that = (TickerDTO)o;
        return (new EqualsBuilder()).append(this.currencyPair, that.currencyPair).append(this.getTimestamp(), that.getTimestamp()).isEquals();
    } else {
        return false;
    }
}

and in the getTimestamp method I seeing this:

public final ZonedDateTime getTimestamp() {
    return (ZonedDateTime)Objects.requireNonNullElseGet(this.timestamp, ZonedDateTime::now);
}

so, if the current and previous ticker had null timestamps, the comparison would be made on the same value ZonedDateTime.now(), is correct?

Thank you.

@straumat
Copy link
Member

@eRfO If timestamp from the server is null the getTimestamp() method will return now which will be different from the ticker timestamp so it should make the new ticker not equal. Can you try 4.1.2-SNAPSHOT ?
My test is working on Binance so it should work for you too.

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat I tried with the 4.1.2-SNAPSHOT but the result is the same:

image

@Tempys
Copy link

Tempys commented Apr 16, 2021

@eRfO @straumat the same for me .

@straumat
Copy link
Member

@eRfO @Tempys I made a simple project from my archetype, I setup my test binance account and I received several tickers. Can you take a look if it works for you ? project source

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat, ok, I requested the access to the project source

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat just tried, the result is the same:

image

@straumat
Copy link
Member

@eRfO you did mvn spring-boot:run on my project ?

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

image

@straumat this is the situation when a ticker is retrieved from binance, the current ticker and the previous one are different but the flux not emit a new ticker to the strategy because both timestamps are null.

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@eRfO you did mvn spring-boot:run on my project ?

I runned your project with IntelliJ.

@Tempys
Copy link

Tempys commented Apr 16, 2021

@straumat i agree with @eRfO because i got 3 events when prices on binances changed 10 or 30 times per few minutes .

@straumat
Copy link
Member

@eRfO can you suppress the tech directory in your .m2 and run mvn spring-boot:run in my project directory ?

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@eRfO can you suppress the tech directory in your .m2 and run mvn spring-boot:run in my project directory ?

@straumat Yes, I suppressed the tech directory and run mvn spring-boot:run the result is the same:

image

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat is this ZonedDateTime.now().equals(ZonedDateTime.now()) deterministic?

@straumat
Copy link
Member

straumat commented Apr 16, 2021

@eRfO It should be... what i don't get is how can the result be different from on my computer and yours... DO you use JDK 11? very strange... maybe we can try to make a live debug tomorrow night on discord?

openjdk version "11.0.10" 2021-01-19
OpenJDK Runtime Environment (build 11.0.10+9-Ubuntu-0ubuntu1.20.04)
OpenJDK 64-Bit Server VM (build 11.0.10+9-Ubuntu-0ubuntu1.20.04, mixed mode, sharing)

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat I'm using JDK 15 but this code ZonedDateTime.now().equals(ZonedDateTime.now()) is not deterministic, if you try this code you will see it :

    for (int i = 0; i < 100; i++) {
        ZonedDateTime now1 = ZonedDateTime.now();
        ZonedDateTime now2 = ZonedDateTime.now();
        boolean equals = now1.equals(now2);
        System.out.printf("%s : %s == %s\n", equals, now1, now2);
        Thread.sleep(5L);
    }
java version "15.0.2" 2021-01-19
Java(TM) SE Runtime Environment (build 15.0.2+7-27)
Java HotSpot(TM) 64-Bit Server VM (build 15.0.2+7-27, mixed mode, sharing)

@straumat
Copy link
Member

@eRfO but in my code, i do call my equal methods that does : equals = ((ZonedDateTime) object1).isEqual((ZonedDateTime) object2); Do you think it's still the problem ?
source

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat yes, I do. If you try to change the test code with isEqual method the result is, again, not deterministic.

@straumat
Copy link
Member

@eRfO what do you think I should change ?

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat I think that you should add other fields in TickerDTO equals method or you could set timestamp when you map the xchange Ticker object with yours only if null.

@straumat
Copy link
Member

@eRfO Ok. i will study this solution but I always prefer to reproduce the problem before fixing it. Is it possible for you to run my test project with a JDK11 ?

@eRfO
Copy link
Author

eRfO commented Apr 16, 2021

@straumat yes, i will do a test with jdk 11, i will let you know.

@straumat straumat modified the milestones: 4.2.0, 4.2.1 Apr 18, 2021
@straumat
Copy link
Member

@eRfO Hi! maybe I know what's happening. Are you asking a lot of currency pairs?

@eRfO
Copy link
Author

eRfO commented Apr 19, 2021

@straumat Hi! No, I just asking for one currency pair. Why are you still in doubt about the problem? We have established that the comparison with the ZonedDateTime.now() is not deterministic, you can try using the simple test that I posted. Furthermore, I think that insert a code inside a simple getter method to return data when that is null is not a best practice.

@straumat
Copy link
Member

@eRfO well... before fixing a problem, I always try to reproduce it in a unit test. I think it's not bad practice as that field is mandatory for me and I retrieve it from an external source.
From my point of view, I still don't get why the same code works on my computer and not on yours. I will add the other fields if I don't find another solution but my idea is first to reproduce in the test then fix it.
I've updated all the libraries, can you make a last try with 4.2.1-SNAPSHOT please? (the snapshot will be available in 20 minutes from now).

@eRfO
Copy link
Author

eRfO commented Apr 19, 2021

@straumat I will try with the new version. In the meantime, I prepared a simple test to reproduce the problem.

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import tech.cassandre.trading.bot.dto.market.TickerDTO;
import tech.cassandre.trading.bot.dto.util.CurrencyPairDTO;

import java.util.LinkedHashMap;
import java.util.Map;

import static tech.cassandre.trading.bot.dto.util.CurrencyDTO.BTC;
import static tech.cassandre.trading.bot.dto.util.CurrencyDTO.EUR;

public class TickerTimestampTest {

    private static Logger logger = LoggerFactory.getLogger(TickerTimestampTest.class);

    @Test
    public void whenTimestampIsNullTest() throws InterruptedException {
        final Map<CurrencyPairDTO, TickerDTO> previousValues = new LinkedHashMap();
        int count = 0;
        for (int i = 0; i < 100; i++) {
            TickerDTO newTicker = TickerDTO.builder().currencyPair(new CurrencyPairDTO(BTC, EUR)).build();
            TickerDTO oldTicker = previousValues.get(newTicker.getCurrencyPair());
            if (!newTicker.equals(oldTicker)) {
                logger.debug("TickerFlux - New ticker received : {}", newTicker);
                previousValues.put(newTicker.getCurrencyPair(), newTicker);
            } else {
                count++;
            }
            Thread.sleep(100L);
        }
        logger.info("Count: {}", count);
        Assertions.assertEquals(0, count);
    }

}

this testifies to the non-deterministic nature of the code.

@eRfO
Copy link
Author

eRfO commented Apr 19, 2021

@straumat I changed the version to 4.2.1-SNAPSHOT but the equals method is the same. What change have you made?

@straumat
Copy link
Member

@eRfO I did not change the equals method yet. But I upgraded lots of libraries (spring, reactor), I set parameters to the schedule threadpools. Now, I will integrate your tests to fix the problem.
I'm still so surprised that the same code have different results on your computer and mine. It also works for @Tempys

@eRfO
Copy link
Author

eRfO commented Apr 19, 2021

@straumat I understood that @Tempys also had the same problem, he wrote this #544 (comment). You shouldn't be surprised that the same code have different results, is in the nature of non deterministic code, that's the point!

@straumat
Copy link
Member

@eRfO Hi, thanks a lot for the tests. I inserted it in my tests and it never fails on my computer. Does it fail every time on yours? Thanks for the help and sorry for the delay.

@eRfO
Copy link
Author

eRfO commented Apr 21, 2021

@straumat Hi, yes it fails every time on my computer.

@straumat straumat reopened this Apr 22, 2021
@straumat straumat removed this from the 4.2.1 milestone Apr 27, 2021
@straumat
Copy link
Member

straumat commented May 2, 2021

@eRfO Hi i made a test with JDK14 and it works well. Besides, even if ZonedDatetime.now returns the same value one time, it should not always do that so you should receive, at least, some tickers. Were you able to make another try ? Thanks

@straumat straumat added the help wanted Extra attention is needed label May 2, 2021
@straumat
Copy link
Member

straumat commented May 6, 2021

The equals has been removed with issue #585

@straumat straumat closed this as completed May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants