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

parseOptions validates args it is not supposed to get for perByte(Trace) functions #224

Open
alexzurbonsen opened this issue Aug 12, 2024 · 5 comments

Comments

@alexzurbonsen
Copy link

alexzurbonsen commented Aug 12, 2024

Describe the bug
When you call perByte or perByteTrace the function parseOptions also validates args that it cannot get (dataReloadRatio) It logs a lot of stuff which can be consufing in the logs since you get messages that do not apply. Also I am not sure it needs logging at all. After all the values are optional in to begin with. <-- this does not happen for perByteTrace options.

To Reproduce
Steps to reproduce the behavior:
Call perByte and observe console.logs.

Expected behavior
No misleading logs or in this case even better: no logs at all.

@fershad
Copy link
Contributor

fershad commented Aug 23, 2024

Hi Alex. Can you please give a code example using both perByte & perByteTrace which returns the logs?

@alexzurbonsen
Copy link
Author

alexzurbonsen commented Aug 23, 2024

Hi fershad, of course, at least for perByteTrace. I am not sure anymore, why I also mentioned perByte - I guess that is wrong - as far as I can see the parseOptions call is responsible for the logging.

swd.perByteTrace(
      1000,
      undefined,
      co2jsOptions,
    )

where co2jsOptions is of

type CO2jsOptions = {
  greenHostingFactor: number;
  gridIntensities: {
    device: number | null;
    dataCenter: number | null;
    network: number | null;
  };
};

I simplified CO2jsOptions a little here.

@fershad
Copy link
Contributor

fershad commented Aug 23, 2024

Cheers Alex. Yep, I can see that now for perByteTrace.

import { co2 } from '@tgwf/co2'
const estimate = new co2({ model: "swd", version: 4 })
const bytesSent = 1000 * 1000 * 1000; // 1GB expressed in bytes

const options = {
    greenHostingFactor: 0,
    gridIntensities: {
      device: 450,
      dataCenter: 209,
    }
  };

const estimatedCO2 = estimate.perByteTrace(bytesSent, undefined, options);
console.log(estimatedCO2);

Running the code below sees the following console output:

The dataReloadRatio option must be a number. You passed in a undefined.
Falling back to default value.
The firstVisitPercentage option must be a number. You passed in a undefined.
Falling back to default value.
The returnVisitPercentage option must be a number. You passed in a undefined.
Falling back to default value.

{
  co2: 148.2,
  green: false,
  variables: {
    description: 'Below are the variables used to calculate this CO2 estimate.',
    bytes: 1000000000,
    gridIntensity: {
      description: 'The grid intensity (grams per kilowatt-hour) used to calculate this CO2 estimate.',
      device: [Object],
      dataCenter: [Object],
      network: [Object]
    },
    greenHostingFactor: 0
  }
}

Those first three lines are the superfluous warning messages that are being returned, despite being not relevant to the perByteTrace function.

@alexzurbonsen
Copy link
Author

Yes, that's it. Thank you for looking into it :)

@fershad
Copy link
Contributor

fershad commented Sep 4, 2024

@alexzurbonsen I won't be able to take a proper look at this until early October. If you've got time and capacity between now & then, we'd welcome a PR which I could review when I'm back from leave.

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

No branches or pull requests

2 participants