brocaar / chirpstack-application-server

ChirpStack Application Server is an open-source LoRaWAN application-server.
https://www.chirpstack.io
MIT License
502 stars 325 forks source link

Revert Fix log duplication on WebSocket re-connect. #622

Closed sagar-patel-sls closed 3 years ago

sagar-patel-sls commented 3 years ago

What happened?

@brocaar We need to revert log duplication on WebSocket re-connect 0518d48 commit.

What did you expect?

All Redis streams records show on the web interface.

Could you share your log output?

  1. DeviceData.js if (data.length === 0 || moment(d.publishedAt).isAfter(data[0].publishedAt)) {}

I have debugged using console.log(data[0].publishedAt) and shown undefined. device-data_err_undefined

  1. DeviceFrames.js

if (frames.length === 0 || moment(f.publishedAt).isAfter(moment(frames[0].publishedAt))) {}

I have debugged using console.log(frames[0].publishedAt shows undefined) and shown undefined. device-frames_err_undefined

  1. GatewayFrames.js if (frames.length === 0 || moment(f.publishedAt).isAfter(moment(frames[0].publishedAt))) {}

I have debugged using console.log(frames[0].publishedAt shows undefined) and shown undefined.

Redis Stream logs (Stored 9 records) redis-stream-logs

Device Data (Shows only 7 records) device-data_err

Resolutions

When I have debugged this issue with the comment code of this commit 0518d48 and checked the result, it's working fine.

onData(d) {
    if (this.state.paused) {
      return;
    }

    let data = this.state.data;
    const now = new Date();

    //console.log(data[0].publishedAt);
    //if (data.length === 0 || moment(d.publishedAt).isAfter(data[0].publishedAt)) {
      data.unshift({
        id: now.getTime(),
        publishedAt: d.publishedAt,
        type: d.type,
        payload: JSON.parse(d.payloadJSON),
      });

      this.setState({
        data: data,
      });
    //}
  }

device-data_resolved

Your Environment

Component Version
Application Server v3.17.0
Network Server v3.15.0
Gateway Bridge v3.13.1
brocaar commented 3 years ago

console.log(data[0].publishedAt);

Did you add this line? Then I think this is the issue. Please see the line below:

if (data.length === 0 || moment(d.publishedAt).isAfter(data[0].publishedAt)) {

data.length could be 0, thus only when data.lenth > 0 you could take the first item using data[0]. That is why you are seeing the undefined error.

Also I don't think we should uncomment this line:

if (data.length === 0 || moment(d.publishedAt).isAfter(data[0].publishedAt)) {

Its purpose is to (entries) to the overview under two conditions:

It is there to avoid duplicated entries on websocket reconnect as both conditions would fail. Please let me know if I'm overlooking something.

sagar-patel-sls commented 3 years ago

Did you add this line? Then I think this is the issue. Please see the line below:

Yes, I have added this console.log(data[0].publishedAt); line and tested but I got the above screenshot error.

I have checked and debug the above two conditions but it fails and getting the above errors. I think we need to think of something different.

Thanks

brocaar commented 3 years ago

But please see your first screenshot, it fails on:

console.log(data[0].publishedAt);

Which can be expected as data is an array and there is no guarantee that it contains items, you are requesting an index that does not exist. You should remove that line first before you can debug this further.

sagar-patel-sls commented 3 years ago

Yes, I have removed the below line and debug but not getting success

console.log(data[0].publishedAt);

I have added the above line for testing purposes.

  1. I have tested without the above line but some of the redis streams not showing on the Device data tab (see section 3: GatewayFrames.js).

  2. After this issue I have removed 0518d48 this commit code and it's working fine.

  3. For debugging purposes I have added console logs to check publishedAt value in the data array that's why I have added the below line. console.log(data[0].publishedAt);

  4. I have verified this issue with the help of the redis-cli and device data tab (See Screenshot: Redis Stream logs (Stored 9 records) & Device Data (Shows only 7 records))

  5. I have attached the first screenshot for your reference.

if you need more information please let me know... Thanks

sagar-patel-sls commented 3 years ago

@brocaar

Any update?

sagar-patel-sls commented 3 years ago

Hi @brocaar

This issue is resolved by this 651b572 commit.