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

Expose device info to webapp via bridge #95

Merged
merged 11 commits into from
Nov 24, 2019

Conversation

tookam
Copy link
Contributor

@tookam tookam commented Oct 22, 2019

Expose device info to webapp via bridge

Example of info shared by code:

{
  "androidVersion":"7.0",
  "osApiLevel":24,
  "osVersion":"3.18.35(1560398223)",
  "device":"Studio_Mega",
  "model":"Studio Mega",
  "manufacturer":"BLU",
  "harware":"mt6580",
  "freeMemorySize":"1,550MB",
  "totalMemorySize":"4,308MB",
  "freeRAMSize":"231MB",
  "totalRAMSize":"894MB",
  "thresholdRAM":"127MB",
  "cpuInfo":"ARMv7 Processor rev 3 (v7l)"
}

#64

MedicAndroidJavascript.java:264: Error: Field requires API level 23 (current min is 19): android.os.Build.VERSION#SECURITY_PATCH [InlinedApi]
     String securityPatch = Build.VERSION.SECURITY_PATCH;
@tookam tookam requested a review from SCdF October 22, 2019 18:59
Copy link
Contributor

@SCdF SCdF left a comment

Choose a reason for hiding this comment

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

Great start! Some changes suggested.

Also, did you also look into:

Network capacity re: their sim card (this is may be hard because we might just catch them at a bad time or whatever. It would have to be a static truth that was representative regardless of when we compiled it).

As well?

@tookam
Copy link
Contributor Author

tookam commented Oct 24, 2019

New json:

{
  "app":{
    "version":"SNAPSHOT"
  },
  "software":{
    "androidVersion":"7.0",
    "osApiLevel":24,
    "osVersion":"3.18.35(1560398223)"
  },
  "hardware":{
    "device":"Studio_Mega",
    "model":"Studio Mega",
    "manufacturer":"BLU",
    "hardware":"mt6580",
    "cpuInfo":{
      "cores":"3",
      "model name":"ARMv7 Processor rev 3 (v7l)"
    }
  },
  "memory":{
    "free":"494MB",
    "total":"4,308MB"
  },
  "ram":{
    "free":"252MB",
    "total":"894MB",
    "threshold":"127MB",
    "maxMemory":"256MB"
  },
  "network":{
    "downSpeed":"1Gbps",
    "upSpeed":"1Gbps"
  }
}

Copy link
Contributor

@SCdF SCdF left a comment

Choose a reason for hiding this comment

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

This looks awesome dude!

One change: can we have any numerical values that we'd want consider numerical for processing reasons as numbers in JSON, as opposed to strings?

As I see it, that's core count, ram usage and network speed. This allows us to more easily process them later.

@tookam
Copy link
Contributor Author

tookam commented Oct 30, 2019

New json:

{
  "app": {
    "version": "SNAPSHOT"
  },
  "software": {
    "androidVersion": "7.0",
    "osApiLevel": 24,
    "osVersion": "3.18.35(1560398223)"
  },
  "hardware": {
    "device": "Studio_Mega",
    "model": "Studio Mega",
    "manufacturer": "BLU",
    "hardware": "mt6580",
    "cpuInfo": {
      "cores": 3,
      "model name": "ARMv7 Processor rev 3 (v7l)"
    }
  },
  "memory": {
    "free": 1555611648,
    "total": 4517351424
  },
  "ram": {
    "free": 233095168,
    "total": 937918464,
    "threshold": 133169152,
    "maxMemory": 268435456
  },
  "network": {
    "downSpeed": 1048576,
    "upSpeed": 1048576
  }
}

@tookam tookam requested a review from SCdF October 30, 2019 18:25
Copy link
Contributor

@SCdF SCdF left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Unfortunately I missed a couple of things that I should have caught in the last review... sorry! I know it's annoying to go back and forth a lot and I should have caught them the first time

long totalRAMSize = memoryInfo.totalMem;
long freeRAMSize = memoryInfo.availMem;
long thresholdRAM = memoryInfo.threshold;
Runtime runtime = Runtime.getRuntime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed when this was suggested so I didn't comment on it.

This may actually be more of a question for @kennsippell since he suggested it.

Because for us the JVM does basically nothing (because most of the work is XWalk -> Chromium which doesn't run in the JVM), does this value and other values in this class indicate useful things for us? If so why?

If it is useful, I wonder then if we want to pull more from it. Things that look interesting to me: availableProcessors, freeMemory, maxMemory (the one we have), totalMemory.

Copy link
Member

@kennsippell kennsippell Oct 31, 2019

Choose a reason for hiding this comment

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

i agree there likely isn't much value in capturing jvm metrics for our app. they are good to look at to confirm nothing unexpected in the usage - but in terms of capturing regularly and per-user it's probably not that useful...

@wtekeu As stefan suggests, I think this should be safe to remove. Would you mind confirming expectations that the Runtime.getRuntime().totalMemory() number is quite small and thus not capturing much of interest?

.put("free", freeRAMSize)
.put("total", totalRAMSize)
.put("threshold", thresholdRAM)
.put("maxMemory", maxMemorySize);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to keep this, but only this from the runtime, it should be jvmMaxMemory or runtimeMaxMemory to avoid confusion with the rest of the system information.

If we decide to keep this and expand to the other information we should have a new section for jvm or runtime information.

@tookam tookam requested a review from SCdF November 6, 2019 22:48
Copy link
Contributor

@SCdF SCdF left a comment

Choose a reason for hiding this comment

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

Awesome, nice work!

@SCdF
Copy link
Contributor

SCdF commented Nov 7, 2019

Now the next step is a cht-core PR to hook this into telemetry, and then we can AT it!

@kennsippell kennsippell removed their request for review November 14, 2019 23:49
@ngaruko ngaruko merged commit 68207c0 into master Nov 24, 2019
@ngaruko ngaruko deleted the 64-expose-device-info-to-webapp branch November 24, 2019 22:56
Copy link
Contributor

@SCdF SCdF left a comment

Choose a reason for hiding this comment

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

Extra notes post merge @wtekeu.

if (data.length > 1) {
String key = data[0].trim();
if (key.equals("model name")) {
output.put(key, data[1].trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you're no longer counting cores this way you don't need to keep looping if you find the "model name" key. In fact, you don't need to loop at all, and instead use indexOf

nijanthanscs pushed a commit to livinggoods/medic-android that referenced this pull request Dec 30, 2020
* Expose device info to webapp via bridge

medic#64
nijanthanscs pushed a commit to livinggoods/medic-android that referenced this pull request Dec 30, 2020
* Expose device info to webapp via bridge

medic#64
nijanthanscs pushed a commit to livinggoods/medic-android that referenced this pull request Dec 30, 2020
* Expose device info to webapp via bridge

medic#64
derickl pushed a commit that referenced this pull request Jan 21, 2021
* Expose device info to webapp via bridge

#64
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.

4 participants